Opened 3 years ago

Last modified 4 months ago

#5573 enhancement new

Add support for AF_UNIX to IReactorSocket.adoptStreamPort

Reported by: exarkun Owned by: getoffmalawn
Priority: normal Milestone:
Component: core Keywords:
Cc: glyph, morgen, cwillu@…, nathan@… Branch: branches/unix-adopt-port-5573-3
(diff, github, buildbot, log)
Author: glyph, getoffmalawn Launchpad Bug:

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 getoffmalawn 4 months ago.
5573-adopt-unix-sockets-v2.patch (8.9 KB) - added by getoffmalawn 4 months ago.
Patch without exception style changes
5573-adopt-unix-sockets-v3.patch (14.8 KB) - added by getoffmalawn 4 months ago.

Download all attachments as: .zip

Change History (21)

comment:1 Changed 3 years ago by exarkun

This will be easier after #5578.

comment:2 Changed 2 years ago by exarkun

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 2 years ago by therve

  • Owner set to therve

comment:4 Changed 2 years ago by therve

  • Author set to therve
  • Branch set to branches/unix-adopt-port-5573

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

comment:5 Changed 2 years ago by glyph

  • Cc glyph morgen added

comment:6 Changed 11 months ago by cwillu

  • Cc cwillu@… added

Changed 4 months ago by getoffmalawn

comment:7 follow-up: Changed 4 months ago by getoffmalawn

  • Keywords review added
  • Owner changed from therve to getoffmalawn
  • Status changed from new to assigned

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 4 months ago by getoffmalawn

  • Author changed from therve to getoffmalawn
  • Cc nathan@… added

comment:9 in reply to: ↑ 7 ; follow-up: Changed 4 months 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 4 months ago by glyph

  • Owner getoffmalawn deleted
  • Status changed from assigned to new

Changed 4 months ago by getoffmalawn

Patch without exception style changes

comment:11 in reply to: ↑ 9 Changed 4 months ago by getoffmalawn

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 follow-up: Changed 4 months ago by exarkun

  • Keywords review removed
  • Owner set to getoffmalawn

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 4 months ago by exarkun

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 4 months ago by getoffmalawn

comment:14 in reply to: ↑ 12 Changed 4 months ago by getoffmalawn

  • 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 4 months ago by glyph

  • Owner getoffmalawn deleted

comment:16 Changed 4 months ago by glyph

  • Author changed from getoffmalawn to glyph, getoffmalawn
  • Branch changed from branches/unix-adopt-port-5573 to branches/unix-adopt-port-5573-3

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

comment:17 Changed 4 months ago by glyph

Patch uploaded and branched kicked off for the next reviewer.

comment:18 Changed 4 months ago by adiroiban

  • Keywords review removed
  • Owner set to getoffmalawn

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!

Note: See TracTickets for help on using tickets.