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

Add Poll class, fix some tests, throttle polling. #6141

Merged
merged 142 commits into from Apr 9, 2019

Conversation

@afshin
Copy link
Member

@afshin afshin commented Mar 26, 2019

Fixes #3929

  • Adds a Poll class to @jupyterlab/coreutils to abstract polling and exponentially backing off in case of failure.
  • Adds Poll class tests.
  • Cleans up doc strings in the running extension.
  • Updates different ad hoc polls to use the Poll class:
    • @jupyterlab/services:KernelManager#models
    • @jupyterlab/services:KernelManager#specs
    • @jupyterlab/services:SessionManager#models
    • @jupyterlab/services:SessionManager#specs
    • @jupyterlab/services:TerminalManager#models
    • @jupyterlab/statusbar:MemoryUsage#metrics
  • Switches kernel manager and session manager to async.
  • A future enhancement of the Poll class that is possible is adding Poll[Symbol.asyncIterator], Poll#return(), and Poll#next() methods to make the poll an async iterator.
  • Updates tests and switches some previously asynchronous tests to synchronous (as long as the semantics are not modified).
  • Re-enables dialog tests in dialog.spec.tsx file that were accidentally disabled and modernizes them to jest API.
  • Cleans up Dialog class API and adds an instance tracker: Dialog.tracker

State diagram of the possible state transitions between poll phases

poll state diagram

@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented Mar 26, 2019

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@afshin afshin changed the title [WIP] Add Poll class, fix tests, throttle polling. Add Poll class, fix tests, throttle polling. Mar 26, 2019
@afshin afshin force-pushed the api-requests branch 2 times, most recently from 75f3829 to 4b9c13a Mar 27, 2019
@afshin afshin requested a review from jasongrout Mar 27, 2019
@afshin afshin force-pushed the api-requests branch 3 times, most recently from c8dbf94 to 33ed638 Mar 28, 2019
@afshin afshin force-pushed the api-requests branch 3 times, most recently from 8ff1be7 to bad17cc Mar 29, 2019
@afshin afshin changed the title Add Poll class, fix tests, throttle polling. [WIP] Add Poll class, fix tests, throttle polling. Mar 29, 2019
@afshin
Copy link
Member Author

@afshin afshin commented Mar 29, 2019

The broken tests are correct. I will remove the [WIP] tag when ready for review.

@afshin afshin force-pushed the api-requests branch 7 times, most recently from 7db2cda to 5c1ea05 Mar 30, 2019
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Apr 8, 2019

I opened afshin#24 for some changes to the kernel manager logic. I'm continuing to work my way through the files in the PR, and I may add more to that PR.

let xsrfToken = getCookie('_xsrf');
if (xsrfToken !== void 0) {
}
if (typeof document !== 'undefined' && document && document.cookie) {
Copy link
Contributor

@jasongrout jasongrout Apr 8, 2019

Choose a reason for hiding this comment

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

Can you elaborate on why you took out the else here?

Copy link
Member Author

@afshin afshin Apr 8, 2019

Choose a reason for hiding this comment

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

This was causing reconnects (after stopping the server and starting again) to fail. I didn't definitively understand why but it seems reasonable if we have both the cookie and the header to use both and it turned out to eliminate a class of failures after reconnect.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Apr 8, 2019

One more PR with changes to the terminal manager: afshin#25

Update terminal manager to be consistent with kernel/session manager async logic
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Apr 8, 2019

For a future PR, I noticed in using the API it might be nice if .refresh() returns the .tick promise, so you can just do await poll.refresh() instead of await poll.refresh(); await poll.tick().

@@ -124,7 +126,7 @@ describe('terminal', () => {
let called = false;
session = await manager.startNew();
manager.runningChanged.connect((sender, args) => {
expect(session.isDisposed).to.equal(false);
expect(session.isDisposed).to.equal(true);
Copy link
Contributor

@jasongrout jasongrout Apr 8, 2019

Choose a reason for hiding this comment

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

Note to self: this may need to be changed back to false after my recent manager updates.

@@ -203,7 +221,7 @@ describe('kernel/manager', () => {
const kernel = await manager.startNew();
const emission = testEmission(manager.runningChanged, {
test: () => {
expect(kernel.isDisposed).to.equal(false);
expect(kernel.isDisposed).to.equal(true);
Copy link
Contributor

@jasongrout jasongrout Apr 8, 2019

Choose a reason for hiding this comment

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

Note to self: this may need to be changed back to false after my recent manager updates.

@afshin
Copy link
Member Author

@afshin afshin commented Apr 8, 2019

For a future PR, I noticed in using the API it might be nice if .refresh() returns the .tick promise, so you can just do await poll.refresh() instead of await poll.refresh(); await poll.tick().

We could potentially do something like:

async refresh(passthrough = false): Promise<void> {
  if (this.isDisposed) {
    return;
  }

  if (this.state.phase === 'constructed') {
    await this.ready;
  }

  if (this.state.phase !== 'refreshed') {
    this.schedule({
      interval: Private.IMMEDIATE,
      payload: null,
      phase: 'refreshed',
      timestamp: new Date().getTime()
    });
  }

  if (passthrough) {
    await this.tick;
  }
}

But I'm not sure. The idea was to keep the API super simple and predictable: you always get the tick you created, the refreshed tick. But it is a little counter-intuitive if all you care about is the result of the refresh roundtrip to a server and not just the instruction to refresh.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Apr 8, 2019

But I'm not sure.

I'm not sure either. I'm happy to leave it as-is, with a simple consistent api, and see if that's okay, or if we want to make it slightly more convenient later.

@afshin
Copy link
Member Author

@afshin afshin commented Apr 8, 2019

The spot you flagged did indeed fail.

Feel free to push directly into afshin:api-requests branch if you want, because I won't be able to merge a PR until tomorrow otherwise.

Screen Shot 2019-04-08 at 11 58 56 PM

@afshin
Copy link
Member Author

@afshin afshin commented Apr 9, 2019

I apparently neglected to git push the doc string commit on my other machine, so I added it now.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Apr 9, 2019

And the tests all pass! Thanks again for all your work on this one, @afshin!

@jasongrout jasongrout merged commit 730333f into jupyterlab:master Apr 9, 2019
9 checks passed
@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Apr 9, 2019

Nice!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants