PR: Don't block on exit #651
Conversation
If the code tries to exit while calling `self.poller.poll`, the app will freeze until `until_dead` seconds have elapsed. This avoids that outcome.
jupyter_client/channels.py
Outdated
| @@ -121,7 +121,7 @@ def _poll(self, start_time: float) -> t.List[t.Any]: | |||
| events = [] | |||
| while True: | |||
| try: | |||
| events = self.poller.poll(int(1000 * until_dead)) | |||
| events = self.poller.poll(1) | |||
There was a problem hiding this comment.
Does it mean we are going to iterate every milllisecond?
There was a problem hiding this comment.
Well a bit longer than that because of the overhead, the time could be adjusted. How does 100ms sounds?
There was a problem hiding this comment.
On second thought, I don't think the loop is necessary at all. I removed it.
jupyter_client/channels.py
Outdated
| @@ -114,30 +114,18 @@ def _poll(self, start_time: float) -> t.List[t.Any]: | |||
| will be an empty list if no messages arrived before the timeout, | |||
| or the event tuple if there is a message to receive. | |||
| """ | |||
| # Check if an event is waiting | |||
| events = self.poller.poll(0) | |||
There was a problem hiding this comment.
Does it ensure we poll at least once?
There was a problem hiding this comment.
If I understand http://api.zeromq.org/2-1:zmq-poll correctly:
If none of the requested events have occurred on any zmq_pollitem_t item, zmq_poll() shall wait timeout microseconds for an event to occur on any of the requested items. If the value of timeout is 0, zmq_poll() shall return immediately. If the value of timeout is -1, zmq_poll() shall block indefinitely until a requested event has occurred on at least one zmq_pollitem_t. The resolution of timeout is 1 millisecond.
A timeout of 0 means that if a message is already there it will be polled.
There was a problem hiding this comment.
A timeout of 0 means that if a message is already there it will be polled.
Thanks, although pyzmq doesn't mention that.
jupyter_client/channels.py
Outdated
|
|
||
| # Wait until timeout | ||
| until_dead = self.time_to_dead - (time.time() - start_time) | ||
| # ensure poll at least once | ||
| until_dead = max(until_dead, 1e-3) |
There was a problem hiding this comment.
What is the point of waiting at least one millisecond?
|
I think that with your changes, this is not needed anymore: |
jupyter_client/channels.py
Outdated
| # Check if an event is waiting | ||
| events = self.poller.poll(0) | ||
| if events: | ||
| return events |
There was a problem hiding this comment.
I don't think we should check if we have an event at the beginning, we should only wait until_dead and then check.
|
@davidbrochart I applied your comments. |
jupyter_client/channels.py
Outdated
| ready = self._poll() | ||
| if ready: | ||
| self._beating = True |
There was a problem hiding this comment.
| ready = self._poll() | |
| if ready: | |
| self._beating = True | |
| # Wait until timeout | |
| self._exit.wait(self.time_to_dead) | |
| # poll(0) means return immediately (see http://api.zeromq.org/2-1:zmq-poll) | |
| self._beating = bool(self.poller.poll(0)) | |
| if self._beating: |
jupyter_client/channels.py
Outdated
| continue | ||
| else: | ||
| elif not self._exit.is_set(): | ||
| # nothing was received within the time limit, signal heart failure | ||
| self._beating = False |
There was a problem hiding this comment.
| self._beating = False |
jupyter_client/channels.py
Outdated
| @@ -107,38 +105,20 @@ def _create_socket(self) -> None: | |||
|
|
|||
| self.poller.register(self.socket, zmq.POLLIN) | |||
|
|
|||
| def _poll(self, start_time: float) -> t.List[t.Any]: | |||
| def _poll(self) -> t.List[t.Any]: | |||
There was a problem hiding this comment.
I don't think we need the _poll() method anymore.
If the code tries to exit while calling
self.poller.poll, the app will freeze untiluntil_deadseconds have elapsed. This avoids that outcome.