Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Enable Thread-Safe sockets with Asyncio using zmq_poller_fd #2065

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

steamraven
Copy link

@steamraven steamraven commented Feb 3, 2025

Request for comments:

NOTE: This REQUIRES the Draft API

This enables Thread-safe sockets (Like RADIO/DISH and SERVER/CLIENT) to be used with asyncio.

Thread-safe sockets do not have a file-descriptor to use to detect events with asyncio. Instead, for each threadsafe socket, a 0MQ poller object is created, associating the socket, and asking for a file-descriptor for the 0MQ poller.

The zmq_poller_fd was added in ZMQ 4.3.2

Outstanding items:

  • CFFI backend
  • Test Cases
  • Benchmarks
  • Handling of POLLIN and POLLOUT - The original 0MQ sockets activate just read, but the poller allows for masking both POLLIN and POLLOUT
  • Testing of a shared Poller for multiple sockets

Please note I am new to this project, so I do not know the conventions for pulls. Please correct me.

@steamraven
Copy link
Author

Related issue: #1139

@minrk
Copy link
Member

minrk commented Feb 4, 2025

Awesome, thanks for working on this! I'll try to review when I have a chance. But in the meantime, one design goal I'd probably go with is not exposing the Poller object or class publicly, and instead implementing it entirely privately within Socket.get(zmq.FD) for thread-safe sockets. Then a socket can have a private self._zmqpoller defined on first access of get(FD) that gets closed explicitly in Socket.close(). Related: there should always be an explicit method to close an object, as close-on-del is unreliable and should produce a ResourceWarning like this one.

@steamraven
Copy link
Author

steamraven commented Feb 7, 2025

not exposing the Poller object or class publicly

I have moved the ZMQPoller object to private backend
I don't believe I can fully hide the poller in the socket, as the poller FD is NOT the socket FD, and there could be implications. One is that the poller allows masking of POLLIN and POLLOUT messages that the asyncio code needs.
Also, the structure of the ZMQ api requires the handle attribute be addressable, meaning it needs to be part of a c struct. Since I can't bundle it completely in the backend socket, I think it needs to be a full object.

So, TLDR: I have removed it from the __all__ api, so has to be explicitly imported.

On a side note, the ZMQ Poller is a first class object in the Draft api and it would be nice to expose it. I wonder what the performance would be if the pyzmq poller can be rewritten to use it, but that is an entirely separate topic.

Related: there should always be an explicit method to close an object, as close-on-del is unreliable

I respectfully disagree on philosophical grounds, but I have updated the __del__ to a close() to get this pushed through

@minrk
Copy link
Member

minrk commented Feb 8, 2025

Thanks!

I don't believe I can fully hide the poller in the socket, as the poller FD is NOT the socket FD, and there could be implications. One is that the poller allows masking of POLLIN and POLLOUT messages that the asyncio code needs.

Is the poller FD still POLLIN-only edge-triggered FD for 'events'? If so, it may behave the same, as long as we register it with POLLIN | POLLOUT in this case. I'm really not sure, I have no experience with these poller FDs. It is going to require quite a bit of testing. This is very new territory, thanks for exploring it.

I respectfully disagree on philosophical grounds, but I have updated the del to a close() to get this pushed through

Thanks for changing it! It's not really a philosophical point, though. An important fact that's not philosophical is that when you del an object, you aren't guaranteed that __del__ is called, and it often isn't. Garbage collection is allowed to happen at an arbitrary later point, and does so when you have things like circular references or even more commonly non-CPython implementations like PyPy. So it's a good rule (for everything Python) that nothing that happens in __del__ should ever only be in __del__, because the language does not permit you to fully control when __del__ is called.

@steamraven
Copy link
Author

As far as I can tell, the poller fd is a read-only fd that edge triggers making the fd ready to read, like the normal socked fd. it triggers when the monitored sockets have events matching their mask. You definitely could just tell poller to trigger on both POLLIN and POLLOUT. You can definitely use this to shoe-horn in a low level so that it is all but invisible, but that doesn't make sense to me.

I will try to work on some test cases and look into cffi.
I don't use Pypy, so once I get cffi done, I'll need someone to test
I'll also see if I can come up with some code people can use to benchmark.

Also, please let me know when I need to rebase

@minrk
Copy link
Member

minrk commented Feb 10, 2025

Part of the reason I was thinking of the FD behavior is that I think this is what libzmq should do itself (and hopefully will when someone has the time and inclination), so making pyzmq look as much as I think libzmq will/should ought to make the changes to pyzmq simplest, and (hopefully) can be removed when upstream someday fixes the bug.

But if you think it's easier/simpler to do it at the zmq.asyncio level, that's okay, too. It should still have the behavior that the poller is created either at first request or (if in zmq.asyncio) at wrapper creation time, and then closed when the socket is closed.

Also, please let me know when I need to rebase

Thanks for asking. Don't worry about rebasing, it's easier to review if you don't. If the PR history gets messy, I can squash at merge time, but no reason to before then.

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.

2 participants