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

Retain restarter instance on stop #536

Merged
merged 1 commit into from
Mar 26, 2020

Conversation

kevin-bates
Copy link
Member

Since late February the js/services tests have been producing a single failure in one of its kernel auto-restart tests. This issue was occurring on all notebook PRs irrespective to change - which implied a changed dependency. Also in late February, we released jupyter_client 6.0.0. This release consisted of commits more than two years ago.

In looking through the commit history (and knowing I might be a culprit 😊) I began going through the commits that seemed possible candidates. Fortunately, the first I picked was the culprit!

It turns out that commit 7a8394d was resetting the self._restarter attribute when stopping the restarter. Although restarts continued working, auto restarts may not have been getting properly performed.

This change retains the restarter instance across stop requests. Since leaks are not commonly reported in normal circumstances, this probably isn't a big deal. Its applications like Kernel or Enterprise Gateway, where large numbers of kernels can be active where this is more problematic. However, since there hasn't been a lot of noise in the two years since that commit was made, I don't think its restoration is something to worry about.

Here's a history of the CI tests (builds 78-87) that first reproduce the issue, work with some proposed changes, then reproduce it again (upon restoring the issue to the test branch), then succeeding with this branch. The two notebook PRs are completely unrelated, one involving server-side changes, the other invoving client-side changes.

@MSeal
Copy link
Contributor

MSeal commented Mar 26, 2020

Alright, that seems like a reasonable argument and trace. Want me to snap a release for the fix?

@MSeal MSeal merged commit ee93516 into jupyter:master Mar 26, 2020
@kevin-bates
Copy link
Member Author

Want me to snap a release for the fix?

That would be great Matthew - thank you!

@MSeal
Copy link
Contributor

MSeal commented Mar 26, 2020

Done 6.1.2

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