Opened 6 years ago

Closed 5 months ago

#5573 enhancement closed fixed (fixed)

Add support for AF_UNIX to IReactorSocket.adoptStreamPort

Reported by: Jean-Paul Calderone Owned by: Glyph
Priority: normal Milestone:
Component: core Keywords:
Cc: Glyph, ~morgen, Carey Underwood, Nathan Hoad Branch: branches/unix-adopt-port-5573-3
branch-diff, diff-cov, branch-cov, buildbot
Author: glyph, getoffmalawn

Description

#5248 adds support for re-using an existing AF_INET or AF_INET6 socket as a listening port. adoptStreamPort should support AF_UNIX as well (and doing so should be simple).

Attachments (3)

5573-adopt-unix-sockets-v1.patch (10.8 KB) - added by Nathan Hoad 3 years ago.
5573-adopt-unix-sockets-v2.patch (8.9 KB) - added by Nathan Hoad 3 years ago.
Patch without exception style changes
5573-adopt-unix-sockets-v3.patch (14.8 KB) - added by Nathan Hoad 3 years ago.

Download all attachments as: .zip

Change History (26)

comment:1 Changed 6 years ago by Jean-Paul Calderone

This will be easier after #5578.

comment:2 Changed 5 years ago by Jean-Paul Calderone

Once this is implemented, the systemd endpoint should work with UNIX sockets. If it does, then the systemd documentation which mentions that UNIX sockets are not supported should be adjusted. Otherwise, re-open #6310 to make it work.

comment:3 Changed 5 years ago by therve

Owner: set to therve

comment:4 Changed 5 years ago by therve

Author: therve
Branch: branches/unix-adopt-port-5573

(In [37573]) Branching to 'unix-adopt-port-5573'

comment:5 Changed 4 years ago by Glyph

Cc: Glyph ~morgen added

comment:6 Changed 3 years ago by Carey Underwood

Cc: Carey Underwood added

Changed 3 years ago by Nathan Hoad

comment:7 Changed 3 years ago by Nathan Hoad

Keywords: review added
Owner: changed from therve to Nathan Hoad
Status: newassigned

I've attached a patch that gets UNIX socket adoption working for adoptStreamPort and adoptStreamConnection. I've had some trouble getting adoptDatagramPort working, but if I get that going I'll attach a patch to #6954.

A couple of notes about the patch:

  • I've changed all the old-style exception raising to new-style. I'm happy to remove that to minimise changes, if desirable.
  • I haven't got any tests for adoptStreamConnection, and I couldn't find any. In my eyes I basically need to start a server outside of Twisted's vision, accept a connection, and then adopt that connection. If anyone has any advice on writing a test like this or has any similar tests, I'd be happy to have another go at it.

comment:8 Changed 3 years ago by Nathan Hoad

Author: thervegetoffmalawn
Cc: Nathan Hoad added

comment:9 in reply to:  7 ; Changed 3 years ago by Glyph

Replying to getoffmalawn:

  • I've changed all the old-style exception raising to new-style. I'm happy to remove that to minimise changes, if desirable.

Yes, please do that. Smaller changes make it through review faster. Those changes will be great, just please put them into a different patch and ticket :).

Thanks!

comment:10 Changed 3 years ago by Glyph

Owner: Nathan Hoad deleted
Status: assignednew

Changed 3 years ago by Nathan Hoad

Patch without exception style changes

comment:11 in reply to:  9 Changed 3 years ago by Nathan Hoad

Replying to glyph:

Yes, please do that. Smaller changes make it through review faster. Those changes will be great, just please put them into a different patch and ticket :).

Thanks!

Sure! No problem, I've attached that patch now. I have a few other syntactic changes to make to twisted.internet.unix to make it Python 3 compatible, so I might bundle them up with those and submit it.

comment:12 Changed 3 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Nathan Hoad

I haven't got any tests for adoptStreamConnection, and I couldn't find any. In my eyes I basically need to start a server outside of Twisted's vision, accept a connection, and then adopt that connection. If anyone has any advice on writing a test like this or has any similar tests, I'd be happy to have another go at it.

There are various existing tests for this

exarkun@top:~/Projects/Twisted/trunk$ grep adoptStreamConnection twisted/ -r --include 'test_*.py'
twisted/internet/test/test_tcp.py:    Test server transports built using C{adoptStreamConnection}.
twisted/internet/test/test_tcp.py:        a C{adoptStreamConnection} against the original server connection.
twisted/internet/test/test_tcp.py:            reactor.adoptStreamConnection(
twisted/internet/test/test_socket.py:    Builder for testing L{IReactorSocket.adoptStreamConnection}
twisted/internet/test/test_socket.py:        An implementation of L{IReactorSocket.adoptStreamConnection} raises
twisted/internet/test/test_socket.py:            reactor.adoptStreamConnection, connection.fileno(), arbitrary,

comment:13 Changed 3 years ago by Jean-Paul Calderone

Also, you can look at the revision that introduced that functionality: https://twistedmatrix.com/trac/changeset/34031 (note that trac doesn't rendered the added files on that page, one of which a new test module, though).

Changed 3 years ago by Nathan Hoad

comment:14 in reply to:  12 Changed 3 years ago by Nathan Hoad

Keywords: review added

Replying to exarkun:

There are various existing tests for this

exarkun@top:~/Projects/Twisted/trunk$ grep adoptStreamConnection twisted/ -r --include 'test_*.py'
twisted/internet/test/test_tcp.py:    Test server transports built using C{adoptStreamConnection}.
twisted/internet/test/test_tcp.py:        a C{adoptStreamConnection} against the original server connection.
twisted/internet/test/test_tcp.py:            reactor.adoptStreamConnection(
twisted/internet/test/test_socket.py:    Builder for testing L{IReactorSocket.adoptStreamConnection}
twisted/internet/test/test_socket.py:        An implementation of L{IReactorSocket.adoptStreamConnection} raises
twisted/internet/test/test_socket.py:            reactor.adoptStreamConnection, connection.fileno(), arbitrary,

Great! Thank you. I've added tests now and submitted a new patch. There is one note that I forgot to mention earlier, in twisted.internet.unix.Server._fromConnectedSocket(), I wasn't sure of the best place to draw a session number from, so I've set it to zero:

    # FIXME: is this a suitable sessionno?
    sessionno = 0
    self = cls(skt, proto, skt.getpeername(), None, sessionno, reactor)

In twisted.internet.tcp.Server._fromConnectedSocket(), the session number is set to the port of the connected peer - given the UNIX socket doesn't have an equivalent, I'm not sure what to do here.

comment:15 Changed 3 years ago by Glyph

Owner: Nathan Hoad deleted

comment:16 Changed 3 years ago by Glyph

Author: getoffmalawnglyph, getoffmalawn
Branch: branches/unix-adopt-port-5573branches/unix-adopt-port-5573-3

(In [44046]) Branching to unix-adopt-port-5573-3.

comment:17 Changed 3 years ago by Glyph

Patch uploaded and branched kicked off for the next reviewer.

comment:18 Changed 3 years ago by Adi Roiban

Keywords: review removed
Owner: set to Nathan Hoad

Thanks for the patch.

Changes looks good but some tests are failing and they need to be fixed before landing. I guess that Unix datagram test should be skipped on Windows.

Please note that I am still a junior reviewer so I might suggest stupid things. Feel free to ignore them.


I am not very happy with

assert unixEnabled, "UNIX support is not present"

In case we want raise I prefer the explicit method

if not unixEnabled:
   raise NotImplementedError('UNIX support is not present')

and write a test to touch this code and check that assertion is raised.

BUT, he code already raises UnsupportedAddressFamily so I think that we should just remove this assert.


All new methods/functions need documentation for all arguments and return value. ex getConnectedClientAndServer and getListeningPort

Due to missing documentation, twistedchecker is failing.


For FIXME/TODO comments we should create a dedicated ticket and add the URL to the ticket in that comment.

For

sessionno = 0

I think that is ok to open a ticket but also we need to document this behaviour. Maybe in Limitations and Known Issues we can mention that for unix sockets sessionno is always 0


self._preexistingSocket needs to be documented in class docstring even if it is private. In this way other developers should be aware of it and prevent an accidental clash with another method.

what is self.repstr ?


I think that newsfile should be a single line as https://github.com/twisted/newsbuilder will rewrap newlines. Also, it should be a .feature and not a .misc as content for .misc is ignored.

.misc tickets are only listed in the also fixed section with only ticket ID mentioned.

You might want to reformulate it as

twisted.internet.posixbase.PosixReactorBase.adoptStreamPort and twisted.internet.posixbase.PosixReactorBase.adoptStreamConnection now supports UNIX SOCK_STREAM socket types.

in test_ServerAddressUNIX

Helper method to test TCP server addresses on either IPv4 or IPv6

I think that this should be UNIX server not TCP


For

    def getListeningPort(self, reactor, factory):
        """
        Get a UNIX port from a reactor, wrapping an already-initialized file
        descriptor.
        """
        if IReactorSocket.providedBy(reactor):
            portSock = socket(AF_UNIX)
            # Code here
        else:
            raise SkipTest("Reactor does not provide IReactorSocket")

maybe we can have it as

    def getListeningPort(self, reactor, factory):
        """
        Get a UNIX port from a reactor, wrapping an already-initialized file
        descriptor.
        """
        if not IReactorSocket.providedBy(reactor):
            raise SkipTest("Reactor does not provide IReactorSocket")

        portSock = socket(AF_UNIX)
        # More code here
            

Instead of

            # self.mktemp() often returns a path which is too long to be used.
            path = mktemp(suffix='.sock', dir='.')

maybe we can have

path = self.getSocketPath()

def getUnixSocketPath(self):
    """
    Return a path which can be used as an Unix socket.
    """
    # self.mktemp() often returns a path which is too long to be used.
    path = mktemp(suffix='.sock', dir='.')

In this way we don't have to duplicate the comment and in case we find out a better way to generate unix socket path we only have to change the code in one place.


I don't know what to say about this

[peerAddress] = server.factory.peerAddresses

it works but in the other Twisted code I see that it uses

peerAddress = server.factory.peerAddresses[0]

Regarding test_ServerAddressUNIX I think that we should do the assertion synchronous.

protocols = []

d.addCallback(lambda result: protocols.append(protocols))
self.runReactor(reactor)

self.assertEqual([(some, things)], protocols)

In this way we make sure that callback is called. Without this the callback might not be called but test will still pass.


Do we want log.msg in tests?

If yes, then please add a comment why we need this and fix the string formating. ie instead of

"string %s" % value

use

"string %s % (value,)

So this patch still needs changes before being merged.

Please check my comments!

Thanks!

comment:19 Changed 9 months ago by exvito

Owner: changed from Nathan Hoad to exvito

All,

I'm volunteering to pick this up where it was left off. Will work from latest patch + review onwards.

Input is welcome.

comment:20 Changed 9 months ago by exvito

PROGRESS:

  • Merged in latest patch.
  • Addressed feedback from Adi Roiban.
  • Added minor fixes to make tests pass on Python 2 and 3 on Linux and Mac OS.
  • Submited PR at https://github.com/twisted/twisted/pull/742
  • Waiting to see test results.
  • Will request review if tests pass.

comment:21 Changed 6 months ago by exvito

Keywords: review added
Owner: exvito deleted

Reviewers,

This has been on the backburner for a while. I've revisited today and while there's still an oddity with github's codecov/patch check that I can't seem to understand fully, other checks are passing and the code is ready for review.

In summary, I picked up a 2 year old patch and adjusted slightly per the previous reviews here and, of course, to ensure tests pass. There's more info and context in the PR at https://github.com/twisted/twisted/pull/742.

Thanks in advance.

comment:22 Changed 5 months ago by Glyph

Keywords: review removed
Owner: set to Glyph

L G T M. I especially appreciate your attention to detail updating the documentation. Thank you for picking up this old patch!

Will merge.

comment:23 Changed 5 months ago by Glyph <glyph@…>

Resolution: fixed
Status: newclosed

In 66db5c7:

Merge pull request #742 from exvito/5573-exvito-unix-adopt-stream

Author: exvito

Reviewer: glyph

Fixes: ticket:5573

twisted.internet.posixbase.PosixReactorBase.adoptStreamPort and twisted.internet.posixbase.PosixReactorBase.adoptStreamConnection now support AF_UNIX SOCK_STREAM sockets

Note: See TracTickets for help on using tickets.