search

cocagne / txdbus

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

Fix sending multiple unix file descriptors #86

Closed dlech closed 3 years ago

dlech commented 5 years ago

This should fix the bug that is causing the build failure in #81

WhyNotHugo commented 5 years ago

Can you please add some tests for this new functionality? Thanks!

cocagne commented 5 years ago

Thanks for jumping on these pull requests Hugo. Once you're satisfied with the result, let me know and I'll publish a new release

WhyNotHugo commented 5 years ago

@cocagne np! Thanks, will do! ^^

dlech commented 5 years ago

Can you please add some tests for this new functionality?

This is not really new functionality. It is fixing a race condition. I'm afraid that I don't know how to write a test that can reliably reproduce a race condition. I was hoping that the fact that it doesn't break any tests would be good enough.

WhyNotHugo commented 5 years ago

Sorry for the lack of reply, I really don't have much to contribute here. If you've confirmed this reliably fixes the issue for you, it looks good enough for me; I'm not honestly into the finer details of this specific codepath.

Maybe @cocagne has better feedback?

cocagne commented 5 years ago

No, I tend to agree with David. Race conditions are very difficult to test against so non-breakage is likely the best we can do.

On Wed, Oct 30, 2019 at 5:26 AM Hugo Osvaldo Barrera < notifications@github.com> wrote:

Sorry for the lack of reply, I really don't have much to contribute here. If you've confirmed this reliably fixes the issue for you, it looks good enough for me; I'm not honestly into the finer details of this specific codepath.

Maybe @cocagne https://github.com/cocagne has better feedback?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/cocagne/txdbus/pull/86?email_source=notifications&email_token=AANMW7AWNBGDSYWQNGKD2H3QRFONXA5CNFSM4HGZPDY2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECTUQQI#issuecomment-547833921, or unsubscribe https://github.com/notifications/unsubscribe-auth/AANMW7AK24JLQMSGKUUF4HLQRFONXANCNFSM4HGZPDYQ .

dlech commented 5 years ago

To be honest, I'm not really happy with this change because it doesn't handle any errors. I just did it to try to fix the unit tests. But, I think we can live without it.

I would like to get #88 (supersedes #81) merged though because it has fixes that are useful to users of the bleak library.

lindblandro commented 4 years ago

I can verify that this pull request fixes a failing unit test test_call_with_two_UNIX_FD_args.