Skip to content

Conversation

@kapetan
Copy link
Contributor

@kapetan kapetan commented Oct 4, 2020

socket.SOCK_NONBLOCK does not exist on macOS, so creating a dbus instance with an unix path fails. This fix uses same socket creation as with tcp. This works on macOS 10.15.6 with dbus 1.12.20, but I don't know if it has any consequences on other platforms.

Running the tests locally with:

DBUS_SESSION_BUS_ADDRESS="unix:path=${DBUS_LAUNCHD_SESSION_BUS_SOCKET}" python3 -m pytest

Where DBUS_LAUNCHD_SESSION_BUS_SOCKET is something like /private/tmp/com.apple.launchd/unix_domain_listener.

Results in two failed tests:

test_signals_with_changing_owners
OSError: [Errno 57] Socket is not connected
test_tcp_connection_with_forwarding
AssertionError: assert 'abstract' in {'path': '/private/tmp/com.apple.launchd/unix_domain_listener'}

If I use path instead of abstract as dictionary key in test_tcp_connection_with_forwarding the test passes.

@acrisci
Copy link
Member

acrisci commented Oct 6, 2020

I can't guarantee MacOS support because there's no way I can test on this platform, but I would like to support platforms without nonblocking sockets. This is a feature only available in Linux >= 2.6.27. So you should add a feature check using platform.system. None of the code should require nonblocking sockets, but it is compatible with them.

@michallowasrzechonek-silvair
Copy link
Contributor

Any platform that supports python3 and asyncio has nonblocking sockets :|

@acrisci
Copy link
Member

acrisci commented Oct 7, 2020

Oh yeah you're right. I misread the python docs. It's just a different way of setting the nonblocking socket in Darwin. I think you have to set O_NONBLOCK with fcntl() after socket creation in that case.

@kapetan
Copy link
Contributor Author

kapetan commented Oct 8, 2020

What's the correct approach here? I can add something like.

flag = fcntl.fcntl(self._sock.fileno(), fcntl.F_GETFL)
fcntl.fcntl(self._sock.fileno(), fcntl.F_SETFL, flag | os.O_NONBLOCK)

It can be hidden behind a feature flag.

@acrisci
Copy link
Member

acrisci commented Oct 8, 2020

I think that's probably already done in the call to sock.setblocking(False) so this is probably fine.

@acrisci
Copy link
Member

acrisci commented Oct 10, 2020

👍

@acrisci acrisci merged commit 0157411 into altdesktop:master Oct 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants