cocagne / txdbus

Native Python implementation of DBus for Twisted
MIT License
61 stars 38 forks source link

Try to avoid race condition when receiving unix fd parameters #81

Closed dlech closed 4 years ago

dlech commented 5 years ago

When using txdbus with BlueZ, there is a race condition when calling the org.bluez.GattCharacteristic1.AcquireWrite method. After sending the request, we receive an org.freedesktop.dbus.Properties.PropertiesChanged signal. First, txdbus.protocol.BasicDBusProtocol.fileDescriptorReceived is called and stores the unix fd, Then txdbus.protocol.BasicDBusProtocol.rawDBusMessageReceived method is called for the PropertiesChanged signal and clears the cached unix fds. Then the return values of AcquireWrite are receviced and the fd ends up being None.

To work around this, we hang on to the unix fds until we receive a message that has a unix fd as a parameter.

cocagne commented 5 years ago

Looks like a good change but Travis caught that it breaks the following unit test: tests.test_client_against_native_bus.UnixFDArgumentsTester.test_call_with_two_UNIX_FD_args Would you mind reviewing the cause of the failure and either augmenting your patch or updating the test if it shouldn't be failing?

dlech commented 5 years ago

Unfortunately, I can't reproduce this error locally. I get a different error instead on the same test:

[ERROR]
Traceback (most recent call last):
  File "/home/david/work/txdbus/tests/client_tests.py", line 2350, in test_call_with_two_UNIX_FD_args
    f2.fileno(),
twisted.internet.error.ConnectionDone: Connection was closed cleanly.

tests.test_client_against_native_bus.UnixFDArgumentsTester.test_call_with_two_UNIX_FD_args

Also, I get this error when running tests locally on the master branch, so the error I am seeing locally is not introduced by this pull request.

I haven't been able to figure out where it is coming from though. I'm guessing that it is some sort of race condition if it works on travis, but on on my local machine.

dlech commented 5 years ago

I think I figured it out. See #86

dlech commented 5 years ago

I'm not super-happy with this workaround. It does fix my use case, but it doesn't fix the problem in general.

The best solution I have come up with so far is modifying twisted to add a callback that returns the message and all file descriptors in a single callback.

Relevant code in twisted:

https://github.com/twisted/twisted/blob/6ac66416c0238f403a8dc1d42924fb3ba2a2a686/src/twisted/internet/unix.py#L155-L223

Or we could maybe subclass twisted.internet.unix.Server somehow.

progerzua commented 5 years ago

@dlech you are awesome! Used your fix to get bleak working.

dlech commented 4 years ago

Superseded by #88.