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

make BlockingPortal.stop sync #334

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

graingert
Copy link
Collaborator

No description provided.

@graingert graingert force-pushed the make-blocking-portal-stop-sync branch from d77d139 to e0ff495 Compare July 3, 2021 22:55
@graingert graingert force-pushed the make-blocking-portal-stop-sync branch from e0ff495 to 2adff23 Compare July 3, 2021 23:11
@graingert graingert requested a review from agronholm July 3, 2021 23:12
@graingert graingert marked this pull request as ready for review July 3, 2021 23:12
Copy link
Owner

@agronholm agronholm left a comment

Choose a reason for hiding this comment

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

If this method is made synchronous, we need to ensure that people dont try to use it from external threads. At the very least, a warning against that should be added to the docstring. The event.set() call should raise a clear enough error but I feel that the event loop thread id member set should be moved to after the event set, to be safe.

Comment on lines 158 to 159
self._stop_event.set()
self._event_loop_thread_id = None
Copy link
Owner

@agronholm agronholm Jul 5, 2021

Choose a reason for hiding this comment

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

This should avoid breakage when called from external threads.

Suggested change
self._stop_event.set()
self._event_loop_thread_id = None
if self._event_loop_thread_id is None:
return
if threading.get_ident() != self._event_loop_thread_id:
raise RuntimeError('This method must be called from the event loop thread')
self._event_loop_thread_id = None
self._stop_event.set()

Co-authored-by: Alex Grönholm <[email protected]>
@@ -154,11 +155,18 @@ async def stop(self, cancel_remaining: bool = False) -> None:
finish before returning

"""
if self._event_loop_thread_id is None:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah this is wrong - we need to handle cancel_remaining

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