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

Show file tree
Hide file tree
Changes from 5 commits
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
5 changes: 5 additions & 0 deletions packages/coreutils/src/poll.ts
View file
Open in desktop
Expand Up @@ -309,6 +309,11 @@ export class Poll<T = any, U = any> implements IDisposable, IPoll<T, U> {
* Refreshes the poll. Schedules `refreshed` tick if necessary.
*
* @returns A promise that resolves after tick is scheduled and never rejects.
*
* #### Notes
* The returned promise resolves after the tick is scheduled, but before
* the polling action is run. To wait until after the poll action executes,
* await the `poll.tick` promise: `await poll.refresh(); await poll.tick;`
Copy link
Member

Choose a reason for hiding this comment

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

👍

*/
refresh(): Promise<void> {
return this.schedule({
Expand Down
73 changes: 18 additions & 55 deletions packages/filebrowser/src/model.ts
View file
Open in desktop
Expand Up @@ -5,7 +5,8 @@ import {
IChangedArgs,
IStateDB,
PathExt,
PageConfig
PageConfig,
Poll
} from '@jupyterlab/coreutils';

import { IDocumentManager, shouldOverwrite } from '@jupyterlab/docmanager';
Expand Down Expand Up @@ -34,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 @@ -87,8 +83,7 @@ export class FileBrowserModel implements IDisposable {
format: 'text'
};
this._state = options.state || null;
this._baseRefreshDuration =
options.refreshInterval || DEFAULT_REFRESH_INTERVAL;
const refreshInterval = options.refreshInterval || DEFAULT_REFRESH_INTERVAL;

const { services } = options.manager;
services.contents.fileChanged.connect(this._onFileChanged, this);
Expand All @@ -103,8 +98,15 @@ export class FileBrowserModel implements IDisposable {
}
};
window.addEventListener('beforeunload', this._unloadEventListener);
this._scheduleUpdate();
this._startTimer();
this._poll = new Poll({
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,
max: 300 * 1000
},
standby: 'when-hidden'
});
}

/**
Expand Down Expand Up @@ -198,7 +200,7 @@ export class FileBrowserModel implements IDisposable {
}
window.removeEventListener('beforeunload', this._unloadEventListener);
this._isDisposed = true;
clearTimeout(this._timeoutId);
this._poll.dispose();
this._sessions.length = 0;
this._items.length = 0;
Signal.clearData(this);
Expand All @@ -225,10 +227,8 @@ export class FileBrowserModel implements IDisposable {
/**
* Force a refresh of the directory contents.
*/
refresh(): Promise<void> {
this._lastRefresh = new Date().getTime();
this._requested = false;
return this.cd('.');
async refresh(): Promise<void> {
await this.cd('.');
}

/**
Expand Down Expand Up @@ -269,9 +269,9 @@ export class FileBrowserModel implements IDisposable {
if (this.isDisposed) {
return;
}
this._refreshDuration = this._baseRefreshDuration;
this._handleContents(contents);
this._pendingPath = null;
this._pending = null;
if (oldValue !== newValue) {
// If there is a state database and a unique key, save the new path.
// We don't need to wait on the save to continue.
Expand All @@ -296,7 +296,6 @@ export class FileBrowserModel implements IDisposable {
this._connectionFailure.emit(error);
return this.cd('/');
} else {
this._refreshDuration = this._baseRefreshDuration * 10;
this._connectionFailure.emit(error);
}
});
Expand Down Expand Up @@ -586,7 +585,7 @@ export class FileBrowserModel implements IDisposable {

// If either the old value or the new value is in the current path, update.
if (value) {
this._scheduleUpdate();
this._poll.refresh();
ian-r-rose marked this conversation as resolved.
Show resolved Hide resolved
this._populateSessions(sessions.running());
this._fileChanged.emit(change);
return;
Expand All @@ -605,38 +604,6 @@ export class FileBrowserModel implements IDisposable {
});
}

/**
* Start the internal refresh timer.
*/
private _startTimer(): void {
this._timeoutId = window.setInterval(() => {
if (this._requested) {
void this.refresh();
return;
}
if (document.hidden) {
// Don't poll when nobody's looking.
return;
}
let date = new Date().getTime();
if (date - this._lastRefresh > this._refreshDuration) {
void this.refresh();
}
}, MIN_REFRESH);
}

/**
* Handle internal model refresh logic.
*/
private _scheduleUpdate(): void {
let date = new Date().getTime();
if (date - this._lastRefresh > MIN_REFRESH) {
void this.refresh();
} else {
this._requested = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

ah, so it actually was doing a debounce, in that if we tried to schedule an update too soon, it would note it and follow up with an update at some point in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it was doing that.

Note, however, that the user clicking refresh() circumvents the polling all-together, so they get immediate feedback.

Copy link
Member

Choose a reason for hiding this comment

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

Right, the original intent was for the auto-poll to not happen too soon after a user or programmatically triggered poll.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then we should be okay with deleting the logic entirely, then. If a user clicking on the refresh button calls the poll refresh, the poll knows about the refresh and its interval starts over again.

}
}

private _connectionFailure = new Signal<this, Error>(this);
private _fileChanged = new Signal<this, Contents.IChangedArgs>(this);
private _items: Contents.IModel[] = [];
Expand All @@ -647,19 +614,15 @@ 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 _requested = false;
private _sessions: Session.IModel[] = [];
private _state: IStateDB | null = null;
private _timeoutId = -1;
private _refreshDuration: number;
private _baseRefreshDuration: number;
private _driveName: string;
private _isDisposed = false;
private _restored = new PromiseDelegate<void>();
private _uploads: IUploadModel[] = [];
private _uploadChanged = new Signal<this, IChangedArgs<IUploadModel>>(this);
private _unloadEventListener: (e: Event) => string;
private _poll: Poll;
}

/**
Expand Down