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.

Sign up for GitHub

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

Jump to bottom

Use new Poll class for filebrowser contents polling. #6305

Merged
merged 7 commits into from May 11, 2019
Merged

Use new Poll class for filebrowser contents polling. #6305

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8a93826
Use new Poll class for filebrowser contents polling.
ian-r-rose May 4, 2019
07d7b4d
Get rid of `scheduleUpdate`, retain minimum refresh time.
ian-r-rose May 4, 2019
e3ee844
Remove logic around MIN_REFRESH interval.
ian-r-rose May 4, 2019
627b1db
Add explanatory note to poll refresh function.
ian-r-rose May 4, 2019
99faafa
Don't await poll.refresh in filebrowser model refresh, as it cancels
ian-r-rose May 4, 2019
be4cc1a
Use poll refresh, fix up test.
ian-r-rose May 4, 2019
6746f7d
Void unhandled promise.
ian-r-rose May 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 2 additions & 15 deletions packages/filebrowser/src/model.ts
View file
Open in desktop
Expand Up @@ -35,11 +35,6 @@ import { showDialog, Dialog } from '@jupyterlab/apputils';
*/
const DEFAULT_REFRESH_INTERVAL = 10000;

/**
* The enforced time between refreshes in ms.
*/
const MIN_REFRESH = 1000;

/**
* The maximum upload size (in bytes) for notebook version < 5.1.0
*/
Expand Down Expand Up @@ -104,13 +99,7 @@ export class FileBrowserModel implements IDisposable {
};
window.addEventListener('beforeunload', this._unloadEventListener);
this._poll = new Poll({
factory: async () => {
const date = new Date().getTime();
if (date - this._lastRefresh < MIN_REFRESH) {
return;
}
return this.refresh();
},
factory: () => this.cd('.'),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the problem is really that this factory function should be async. It's not waiting for the cd to finish.

Copy link
Contributor

@jasongrout jasongrout May 4, 2019 •
edited

Choose a reason for hiding this comment

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

No, that was an incorrect comment. It is returning a promise, so that should be good.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is async, since this.cd('.') returns a promise. In some of my testing it was explicitly marked as async, which made no difference.

frequency: {
interval: refreshInterval,
backoff: true,
Expand Down Expand Up @@ -239,8 +228,7 @@ export class FileBrowserModel implements IDisposable {
* Force a refresh of the directory contents.
*/
refresh(): Promise<void> {
this._lastRefresh = new Date().getTime();
return this.cd('.');
return this._poll.refresh();
Copy link
Contributor

Choose a reason for hiding this comment

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

You probably want to refresh, then return the poll's tick promise. The refresh just schedules the refresh, the actual action happens at the end of the tick.

Copy link
Contributor

@jasongrout jasongrout May 4, 2019 •
edited

Choose a reason for hiding this comment

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

Or just make an async function like the tests:

await this._poll.refresh();
await this._poll.tick;

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, thanks, I had misunderstood the function signature.

Copy link
Contributor

Choose a reason for hiding this comment

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

We discussed making the refresh return the tick promise, but then it would be inconsistent with the other scheduling functions like start, etc. It would be convenient, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should at least make a note in the refresh method about this subtlety.

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like Poll has some logic around pending requests in which new calls to refresh() supersede older ones. That is not really what we want here, since calls to FileBrowserModel.refresh() can depend on a previous cd (which is what #5224 was fixing). I suspect this is a case that is not really intended to be addressed by the Poll functionality, where invocations of its actions are more-or-less memory-less.

We can get around this by not calling poll.refresh in model.refresh, and relying on the custom logic in cd around pending paths, but it's not particularly clear.

Copy link
Contributor

Choose a reason for hiding this comment

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

Roughly, the order I think should be (IIRC):

  • Call poll.refresh
  • poll.refresh calls schedule async
  • poll.schedule resolves the current poll state (awaiting current tick promise resolve, which should happen after every other thing that was waiting on the current tick)
  • poll.schedule then requests an animation frame to run the poll action (but does not await it).
  • poll.schedule returns

At this point the await poll.refresh should return, and you await the poll.tick promise. The animation frame triggers, which awaits the poll action, then poll.schedules a 'resolved' state for the normal poll interval, and as part of the poll schedule, it resolves the poll.tick promise you were awaiting. The poll.tick promise is resolved with the poll instance, so you can check the poll state (which should be the 'resolved' state that was scheduled).

Does that help?

Copy link
Contributor

Choose a reason for hiding this comment

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

so (await poll.tick()).state.payload should be the value returned from your factory function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Roughly, the order I think should be (IIRC):

* Call poll.refresh

This was happening as expected.

* poll.refresh calls schedule async

Also happening as expected.

* poll.schedule resolves the current poll state (awaiting current tick promise resolve, which should happen after every other thing that was waiting on the current tick)

I don't think this is occurring as described, in particular, the resolution of the current tick does not seem to wait on the factory function:

jupyterlab/packages/coreutils/src/poll.ts

Lines 400 to 403 in 8c72b12

// Emit ticked signal, resolve pending promise, and await its settlement.
this._ticked.emit(this.state);
pending.resolve(this);
await pending.promise; 

* poll.schedule then requests an animation frame to run the poll action (but does not await it).

Agreed.

* poll.schedule returns

Agreed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is occurring as described, in particular, the resolution of the current tick does not seem to wait on the factory function:

The poll action happens in the execute, and after the factory is run and the result resolves, then schedule is called to schedule a resolve state. That's why the tick happens after the factory runs.

}

/**
Expand Down Expand Up @@ -625,7 +613,6 @@ export class FileBrowserModel implements IDisposable {
private _pending: Promise<void> | null = null;
private _pendingPath: string | null = null;
private _refreshed = new Signal<this, void>(this);
private _lastRefresh = -1;
private _sessions: Session.IModel[] = [];
private _state: IStateDB | null = null;
private _driveName: string;
Expand Down