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
Use new Poll class for filebrowser contents polling. #6305
Changes from all commits
8a93826
07d7b4d
e3ee844
627b1db
99faafa
be4cc1a
6746f7d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,8 @@ import { | |
IChangedArgs, | ||
IStateDB, | ||
PathExt, | ||
PageConfig | ||
PageConfig, | ||
Poll | ||
} from '@jupyterlab/coreutils'; | ||
|
||
import { IDocumentManager, shouldOverwrite } from '@jupyterlab/docmanager'; | ||
|
@@ -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 | ||
*/ | ||
|
@@ -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); | ||
|
@@ -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('.'), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is async, since |
||
frequency: { | ||
interval: refreshInterval, | ||
backoff: true, | ||
max: 300 * 1000 | ||
}, | ||
standby: 'when-hidden' | ||
}); | ||
} | ||
|
||
/** | ||
|
@@ -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); | ||
|
@@ -225,10 +227,9 @@ 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._poll.refresh(); | ||
await this._poll.tick; | ||
} | ||
|
||
/** | ||
|
@@ -269,9 +270,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. | ||
|
@@ -296,7 +297,6 @@ export class FileBrowserModel implements IDisposable { | |
this._connectionFailure.emit(error); | ||
return this.cd('/'); | ||
} else { | ||
this._refreshDuration = this._baseRefreshDuration * 10; | ||
this._connectionFailure.emit(error); | ||
} | ||
}); | ||
|
@@ -586,7 +586,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(); | ||
void this._poll.refresh(); | ||
this._populateSessions(sessions.running()); | ||
this._fileChanged.emit(change); | ||
return; | ||
|
@@ -605,38 +605,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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it was doing that. Note, however, that the user clicking There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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[] = []; | ||
|
@@ -647,19 +615,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; | ||
} | ||
|
||
/** | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍