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

Improve shadowing, support async sockets/callbacks in ZMQStream #1785

Merged
merged 7 commits into from
Sep 28, 2022

Conversation

minrk
Copy link
Member

@minrk minrk commented Sep 27, 2022

  • can now create alternate socket APIs for existing sockets by passing one socket to another Socket constructor, with e.g. zmq.asyncio.Socket(socket). Also works with shadow: zmq.Socket.shadow(other_socket) instead of needing shadow(socket.underlying). This also preserves a reference to the shadowed object, avoiding close on garbage collection, which could happen if you tried to do e.g. socket = zmq.asyncio.Socket.shadow(ctx.socket(zmq.PUSH).underlying)
  • add socket_class argument to ctx.socket, e.g. ctx.socket(zmq.PUSH, socket_class=zmq.asyncio.Socket) for easier creation of explicit socket types in a multi-socket-type situation
  • Support async functions in ZMQStream callbacks
  • Support async sockets in ZMQStream (by explicitly shadowing with a sync Socket, no matter what kind of Socket it is)

The two seemingly unrelated changes are together because the async socket->sync socket shadow in zmqstream relies on the reference held when shadowing a socket object.

cc @blink1073

In particular, this has consequences for anything that was relying on the fact that on_recv callbacks were passed a Future. This fixes that in that it will get the right argument now, but it means anything that had adopted and assumed the current (wrong) behavior will need to be updated. I'm not sure what the right way to 'fix' this is without breaking anything that assumed the broken behavior.

there are attributes other than socket options
- accept Context/Socket object as shadow arguments
- accept shadow _objects_ to constructors, so you can do `zmq.Socket(asyncio_socket)`
- when passing objects to shadow, preserve reference to prevent garbage collection of original
for easier construction of custom sockets
# We know async sockets don't work,
# but other socket subclasses _may_.
# should we allow that?
# TODO: warn here?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe explicitly override if the socket subclasses the asyncio socket, and warn otherwise?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT about warning in the async case? This is where behavior is changing, so a warning makes sense. But also now that shadowing is added, it works fine if the new behavior is what you expect, so a warning might be annoying!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe instead error out if the original socket is async but the callback is not? That way you're exposing the previously silent bad behavior.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, that's a little tricky since on_recv is called later, and by then the socket has been cast to sync already.

You also can't tell for sure whether a callback is truly async without calling it. (it could be a sync function e.g. via a decorator that schedules an async handler, which is fine, or calls future.add_done_callback).

Maybe the best is to always warn/cast, and make explicit: ZMQStream only accepts zmq.Socket. If you give it anything else, it'll cast to sync, but will ask you to do the casting for next time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, that works

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went with explicit warning for async sockets, so if it casts, it warns.

Decided to ignore random custom socket classes, since they are rare, and likely implement custom serialization rather than something async. If they are subclasses of the async sockets, it'll the warning path.

Not sure 100%

avoids problems with fast tests
Copy link
Contributor

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@minrk minrk merged commit 0bd2de4 into zeromq:main Sep 28, 2022
@minrk minrk deleted the better-shadow branch September 28, 2022 07:18
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