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

Close new event loops in just_run helper #159

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

Conversation

takluyver
Copy link
Member

I was getting some __del__ errors from a test suite of a project using nbclient:

Error traceback
Exception ignored in: <function BaseEventLoop.__del__ at 0x7f4616d54940>
Traceback (most recent call last):
  File "/usr/lib64/python3.9/asyncio/base_events.py", line 683, in __del__
    self.close()
  File "/usr/lib64/python3.9/asyncio/unix_events.py", line 58, in close
    super().close()
  File "/usr/lib64/python3.9/asyncio/selector_events.py", line 92, in close
    self._close_self_pipe()
  File "/usr/lib64/python3.9/asyncio/selector_events.py", line 99, in _close_self_pipe
    self._remove_reader(self._ssock.fileno())
  File "/usr/lib64/python3.9/asyncio/selector_events.py", line 277, in _remove_reader
    key = self._selector.get_key(fd)
  File "/usr/lib64/python3.9/selectors.py", line 191, in get_key
    return mapping[fileobj]
  File "/usr/lib64/python3.9/selectors.py", line 72, in __getitem__
    fd = self._selector._fileobj_lookup(fileobj)
  File "/usr/lib64/python3.9/selectors.py", line 226, in _fileobj_lookup
    return _fileobj_to_fd(fileobj)
  File "/usr/lib64/python3.9/selectors.py", line 42, in _fileobj_to_fd
    raise ValueError("Invalid file descriptor: {}".format(fd))
ValueError: Invalid file descriptor: -1

I believe this comes from event loops being created to run nbclient async code, and then never cleaned up. #58 might also be related, but I'm not sure of that.

For Python 3.7 and above, it's easy to use asyncio.run(), which deals with creating an event loop and closing it again. For Python 3.6, I've added a try/finally to close the event loop after it finishes.

In theory there's a slight downside that we have to create a new event loop for each synchronous call. However, I think the typical use case (at least, my use case ;-) ) is to make a single execute() call to execute an entire notebook, in which case the overhead of creating and tearing down the event loop is probably negligible.

@takluyver
Copy link
Member Author

Sigh OK, that's not as simple as it looks. Using run() apparently breaks any later calls to get_event_loop() because... something something global state. 😞 The same is true of using .get_event_loop and then .close().

I think we could:

  1. Use new_event_loop to get a temporary event loop without making it the 'current' event loop. I'm not sure what the effect of that is - it would still become the running loop while it runs. I'll give this a try.
  2. Use get_event_loop to use the 'current' event loop, and try to somehow ensure it's cleaned up properly (like in run()). This is hard for a library to get right, because it's meant to be an application thing.

@takluyver
Copy link
Member Author

Ugh, now there are random test failures on a parallel test. 😞

@davidbrochart
Copy link
Member

test_many_parallel_notebooks has always been problematic, but test_allow_errors also fails. It seems to be caused by ZMQ sockets, so I'm not sure it is related to this PR.

@takluyver
Copy link
Member Author

I suspect it's not directly related, but maybe creating new event loops makes it more likely. I confess I restarted CI twice on the last commit in the hope that the random error would go away, but 3 times in a row there has been one job failing (and not the same one), which suggests that it's made the odds worse.

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