Support more than one file descriptor per UNIX socket recvmsg.

[exvito]

See ticket at https://twistedmatrix.com/trac/ticket/8911

[codecov-io]

Current coverage is 90.67% (diff: 100%)

Merging #599 into trunk will decrease coverage by 0.53%

@@              trunk       #599   diff @@
==========================================
  Files           836        813     -23   
  Lines        146451     145371   -1080   
  Methods           0          0           
  Messages          0          0           
  Branches      12986      12834    -152   
==========================================
- Hits         133569     131810   -1759   
- Misses        10650      11294    +644   
- Partials       2232       2267     +35   

Powered by Codecov. Last update dc9880d...fd3811c

[exarkun]

Thanks. Can you add some test coverage?

[exvito]

(preliminary note: not sure if I should be commenting here on in the trac ticket; I'll go ahead and do it here, but please correct me if it should be done in trac)

Thanks for the quick review and feedback, I'll be happy to contribute in improved test coverage given that the code is tested by the existing tests but no tests are in place to test receiving more than one file descriptor per recvmsg.

Like I commented on the track ticket description, I'd appreciate some guidance. Key reasons:

• My working scenario is somewhat complex with two in-development branches (txdbus + Twisted) and an external dependency (dbus), which of course I'll need to isolate from.
• Not being familiar with low-level Twisted testing.
• Looking at the existing tests in src/twisted/internet/test/test_unix.py, in particular test_sendFileDescriptor, I see that fileDescriptorReceived is tested back-to-back with sendFileDescriptor which, for the case this PR is addressing, does not help because sendFileDescriptor sends one file descriptor per sendmsg.

I feel that a testing approach would require manually creating a socket and calling sendmsg on it with more than one file descriptor in ancilliary data and have class similar to ReceiveFileDescriptor handle the receiving side to check the results -- it seems pretty low-level. Maybe there is a better approach.

Could you or anyone please share your thoughts?
Again thanks.

[exarkun]

I feel that a testing approach would require manually creating a socket and calling sendmsg on it with more than one file descriptor in ancilliary data and have class similar to ReceiveFileDescriptor handle the receiving side to check the results -- it seems pretty low-level. Maybe there is a better approach.

That seems like a reasonable approach. The other approach I can think of would involve changing sendFileDescriptor so that it can send more than one file descriptor at a time.

As I recall, the implementation is the way it is now because we could not find a reliable way to deal with more than one file descriptor at a time. Looking at your change, I'm not sure why this was the case - it seems straightforward enough. I may be misremembering the history; it has been quite a while since this work was done.

Looking at the current implementation of sendFileDescriptor and writeSomeData, it seems like a straightforward transformation to encode multiple descriptors in to one sendmsg call. Perhaps there is some challenge around finding a suitable number to encode (considering that you might run into a full buffer situation) but even that doesn't seem too complicated (just try sending fewer if you encounter this, unless you're already trying to send just one, and then do the same thing as the code does now).

Testing sending and receiving separately from each other in addition to testing Twisted's own round-tripping seems like a good thing. Twisted will always follow some pattern in how it sends file descriptors. Hand-crafted sendmsg calls are the most obvious way to test other patterns that may be used by other software.

[exvito]

I feel that a testing approach would require manually creating a socket and calling sendmsg on it with more than one file descriptor in ancilliary data and have class similar to ReceiveFileDescriptor handle the receiving side to check the results -- it seems pretty low-level. Maybe there is a better approach.

That seems like a reasonable approach. The other approach I can think of would involve changing sendFileDescriptor so that it can send more than one file descriptor at a time.

Agreed. For now I'd rather focus on one issue at a time, deferring sendFileDescriptor changes to a future opportunity.

Testing sending and receiving separately from each other in addition to testing Twisted's own round-tripping seems like a good thing. Twisted will always follow some pattern in how it sends file descriptors. Hand-crafted sendmsg calls are the most obvious way to test other patterns that may be used by other software.

Also agreed - multi file descriptor passing round-trip testing will be possible once sendFileDescriptor learns that trick.

As for going with handcrafted sendmsg calls, I'd ask for your guidance in one particular aspect (please correct me if I'm wrong): given that the reactor is running during the tests, which approach would you recommend to handcrafting sendmsg calls? I don't think creating a socket manually and calling sendmsg on it would be compatible with the running reactor, as that call can block... On the other hand, assuming it's possible to grab the underlying socket from a transport (not sure), calling sendmsg on that sounds like a bad idea... Another option, which I don't like much, again, is running the manual socket+sendmsg in a separate thread. I'm probably not seeing the full picture here... (I tried browsing some tests, but couldn't find any suitable example) Any recommendation?

Thanks in advance, again, for some hand-holding in this -- I'd rather have a good idea of where to go before starting coding a test than going back and forth with non-guided attempts.

[exarkun]

I don't think creating a socket manually and calling sendmsg on it would be compatible with the running reactor, as that call can block...

I think this is actually the easiest way to go. My assumption is that the send buffer is big enough to handle anything we might want to test. If that's wrong, we might have to do something more complicated. So, if the assumption holds:

Put the sockets in non-blocking mode. Write what you need to write to set up the case you want to exercise. If the write fails with some kind of buffer-full error (EWOULDBLOCK, EAGAIN, etc), error out the test and someone will just have to work harder to exercise the case. But if the write succeeds, as I assume it will (and reliably so, not just most of the time), then proceed to kick off the reactor-based work: the bytes are already in the kernel send buffer somewhere; nothing further is required on the sending side to finish the scenario.

It's true this is all pretty gross low-level stuff but I'm not sure there's any avoiding that while testing sendmsg-interacting code.

[tomprince]

One question i have, is whether there is might be information being lost in the fact that multiple FDs were sent as a single message.

Although, a reasonable way to handle that would be to add a new interface that accepts multiple FDs from a single message. And then, if that interface isn't provided by the protocol, fall back to the current interface and just send the FDs one-by-one. If that is the final desired solution, then that improvement can be added incrementally later.

[exarkun]

One question i have, is whether there is might be information being lost in the fact that multiple FDs were sent as a single message.

Yea, that's a reasonable concern. Without thinking about it too hard, a new interface with graceful fallback sounds like a reasonable solution, too.

[exvito]

Here's the test I've come up with.

Following @exarkun guidance, a socketpair is created. One end is associated with a FakeReceiver + FakeProtocol, the other end is used to call a crafted sendmsg with two FDs in ancillary data.

It verifies two things and is passing on my system (let's see what CI says about other platforms):

• That fileDescriptorReceived is called twice.
• That the received FDs are not the same as the ones sent.

Things I'm not sure about:

• FakeReceiver approach: creating a Server seemed more complex, so for me it was a shortcut.
• Maybe the docstring can use some improvement. I'm not familiar with the {} notation...

Final notes:

• I ended up using the socket pair FDs with sendmsg, since they were at hand.
• I took care to close all FDs so there should be no leaks.

As before, feedback is very welcome.
Thanks for your guidance.

PS: Re-added review keyword to ticket (that is the process, correct?)
PPS: My first approach was creating a FakeSocket, somewhat inspired in some tests in test_tcp.py, but that ends not being possible in Python 2 due to the C-level send/recvmsg implementations that require real sockets.

[exarkun]

adoptStreamConnect might be just as easy as this, without the abstraction violation.

https://twistedmatrix.com/documents/15.5.0/api/twisted.internet.interfaces.IReactorSocket.html#adoptStreamConnection

[exvito]

Thanks for the prompt feedback. I didn't think of that and it sounds like a very clean approach... Unfortunatelly it seems adoptStreamConnection does not support AF_UNIX sockets (and fails if I lie saying it is AF_INET!).

[glyph]

The appropriate thing to do here would be to have this workaround for now, but also include a link to the "add AF_UNIX to adoptStreamConnection" ticket. I can't find one - the closest is https://twistedmatrix.com/trac/ticket/5573 which is for adoptStreamPort.

[exvito]

@exarkun, I just pushed two minor changes not related to your note: the adoptStreamConnection solution does not seem applicable here, unfortunatelly.

If you recommend I setup a proper Server instead of that FakeReceiver, in the test (or some other solution), I'll happily go after that. Just point me in the right direction.

Thanks.

[exarkun]

the adoptStreamConnection solution does not seem applicable here, unfortunatelly.

Ah, that sucks. There's no real reason why AF_UNIX shouldn't be supported. It looks like someone got this started but the work stalled: https://twistedmatrix.com/trac/ticket/5573

[exvito]

I finally got the tests to pass on Appveyor - I naively thought none of the test_unix.py would even be attempted on Windows, but that wasn't the case.

As for the actual test code (and implementation, of course!), I'll standby on feedback.

[exvito]

Alternative test approach in https://github.com/exvito/twisted/tree/8911-exvito-explore-tests

• No longer violates abstractions.
• Risk of manual socket connect/sendmsg blocking still theoretically there.

Now standing by. Thanks. :)

[exvito]

Updated the branch following the merge from trunk by @hawkowl: AppVeyor seems to have timed out and, thus, failed. Is there any way of having it run the tests again? (other than pushing to this branch, again?)

Thanks.

[glyph]

@exvito Unfortunately appveyor is a little primitive in the way that it interacts with github; pushing is pretty much the only way to get it to fire again. However, there are always new trunk commits coming in so hopefully this time we'll get luckier with appveyor's infra ;)

[glyph]

Since this is exactly the kind of code which is very platform-finicky, I pushed a branch into the twisted/twisted namespace so that we can get the buildbots trying it out on BSD and so on.

[glyph]

This looks like a very clean fix. Assuming we get a clean bill of health on the buildbots, it can be merged with these minor changes. @exvito - please push said changes to your branch and alert a committer that it should be merged (a full re-review would be overkill at this point).

[glyph]

The appropriate thing to do here would be to have this workaround for now, but also include a link to the "add AF_UNIX to adoptStreamConnection" ticket. I can't find one - the closest is https://twistedmatrix.com/trac/ticket/5573 which is for adoptStreamPort.

[glyph]

As per https://jml.io/pages/test-docstrings.html we should remove "verify that" from the docstring.

[glyph]

As per http://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles this should give a fully-qualified Python name as a subject; so, in this case, the affected public APIs would be twisted.internet.unix.Server.doRead and twisted.internet.unix.Client.doRead

[exvito]

@glyph, thanks for your input:

• Updated the PR per your suggestions.
• In fact, even though https://twistedmatrix.com/trac/ticket/5573 says "Add support for AF_UNIX to IReactorSocket.adoptStreamPort" it does include a patch implementing both adoptStreamPort and adoptStreamConnection (see comment 7) - for this reason, I included a link to this ticket in the test comment recommending replacing the current approach with one based on adoptStreamConnection, once it lands.
• Not being familiar with tracking buildbot progress, could you please point out where I can check the associated results?

Lastly, per @glyph's request: dear committers, please merge this in. Thanks.

[exvito]

@glyph, @exarkun or @hawkowl,

Trying to figure out if this PR is "green" on platforms other than the ones tested by AppVeyor and Travis, I went looking for the buildbot results.

Can one of you please help me out understanding them? I can't seem to find this PR's test in the stdio log of the builds here: https://buildbot.twistedmatrix.com/boxes-all?branch=trunk&num_builds=10

Maybe I'm looking in the wrong place or maybe I need to pick a particular build. If so, I would I know which?

Thanks in advance.