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

fix mixture of sync/async sockets in IOPubThread #1275

Merged
merged 5 commits into from
Oct 26, 2024

Conversation

minrk
Copy link
Member

@minrk minrk commented Oct 21, 2024

all sockets are explicitly sync until/except we are in the coroutines that will await them, whereas there was an ambiguous mixture of sync and async sockets before

  • consistent behavior of send for child pipe and main process sockets
  • avoids unsafe assumption that send is greedy on async sockets
  • avoids potential issues creating async objects in one thread, then using them in another in a different event loop
  • always creates/uses the right types, regardless of input socket
  • address some typing lint

found while working on ipython/ipyparallel#895

all sockets are explicitly sync until/except we are in the coroutines that will await them

- consistent behavior of send for child pipe and main process sockets
- avoids unsafe assumption that send is greedy on async sockets
- avoids potential issues creating async objects in one thread, then using them in another in a different event loop
- always creates/uses the right types, regardless of input socket
- address some typing lint
instead, keep same context but use `socket_class` kwarg to specify socket classes

shadow context prevents cleanup of untracked sockets via ctx.destroy because it disconnects socket bookkeeping
@minrk
Copy link
Member Author

minrk commented Oct 22, 2024

ok, actually all correct now, I think

@Carreau Carreau merged commit bf10447 into ipython:main Oct 26, 2024
28 of 32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants