From 3106c54a943bad7049f38cafa671e019bfdd55ca Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Tue, 21 Apr 2020 16:27:27 -0700 Subject: [PATCH 1/3] Change CDN files to download locally (#11286) * Working downloader * Tests passing * Add more tests * Add retry test * Fix build errors. Not sure why not happening locally. * Fix unit tests * Fix azure ml on windows Rework error checking to use status * Fix sonar error * Add some more descriptive comments * More comments * Refactor some code * Make sure to not use exists as we're downloading anyway --- news/2 Fixes/11274.md | 1 + src/client/datascience/constants.ts | 1 + .../cdnWidgetScriptSourceProvider.ts | 210 +++++++++++++-- .../ipywidgets/ipyWidgetMessageDispatcher.ts | 11 +- .../ipywidgets/ipyWidgetScriptSource.ts | 54 ++-- .../ipyWidgetScriptSourceProvider.ts | 9 +- .../localWidgetScriptSourceProvider.ts | 4 +- .../datascience/jupyter/jupyterSession.ts | 6 +- src/client/datascience/types.ts | 4 + src/datascience-ui/ipywidgets/container.tsx | 2 +- ...cdnWidgetScriptSourceProvider.unit.test.ts | 242 ++++++++++++------ ...ipyWidgetScriptSourceProvider.unit.test.ts | 1 - .../jupyter/jupyterSession.unit.test.ts | 4 +- 13 files changed, 412 insertions(+), 137 deletions(-) create mode 100644 news/2 Fixes/11274.md diff --git a/news/2 Fixes/11274.md b/news/2 Fixes/11274.md new file mode 100644 index 000000000000..d179a5a5592e --- /dev/null +++ b/news/2 Fixes/11274.md @@ -0,0 +1 @@ +Fix issue where downloading ipywidgets from the CDN might be busy. \ No newline at end of file diff --git a/src/client/datascience/constants.ts b/src/client/datascience/constants.ts index 8090d3137e1c..72516e9a18bf 100644 --- a/src/client/datascience/constants.ts +++ b/src/client/datascience/constants.ts @@ -404,6 +404,7 @@ export namespace Identifiers { export const InteractiveWindowIdentity = 'history://EC155B3B-DC18-49DC-9E99-9A948AA2F27B'; export const InteractiveWindowIdentityScheme = 'history'; export const DefaultCodeCellMarker = '# %%'; + export const DefaultCommTarget = 'jupyter.widget'; } export namespace CodeSnippits { diff --git a/src/client/datascience/ipywidgets/cdnWidgetScriptSourceProvider.ts b/src/client/datascience/ipywidgets/cdnWidgetScriptSourceProvider.ts index b7e69cfc317c..a54d22f6a451 100644 --- a/src/client/datascience/ipywidgets/cdnWidgetScriptSourceProvider.ts +++ b/src/client/datascience/ipywidgets/cdnWidgetScriptSourceProvider.ts @@ -3,16 +3,25 @@ 'use strict'; -import { traceWarning } from '../../common/logger'; +import * as fs from 'fs-extra'; +import { sha256 } from 'hash.js'; +import * as path from 'path'; +import request from 'request'; +import { Uri } from 'vscode'; +import { traceError, traceInfo } from '../../common/logger'; +import { IFileSystem, TemporaryFile } from '../../common/platform/types'; import { IConfigurationService, IHttpClient, WidgetCDNs } from '../../common/types'; -import { StopWatch } from '../../common/utils/stopWatch'; -import { sendTelemetryEvent } from '../../telemetry'; -import { Telemetry } from '../constants'; +import { createDeferred, sleep } from '../../common/utils/async'; +import { ILocalResourceUriConverter } from '../types'; import { IWidgetScriptSourceProvider, WidgetScriptSource } from './types'; // Source borrowed from https://github.com/jupyter-widgets/ipywidgets/blob/54941b7a4b54036d089652d91b39f937bde6b6cd/packages/html-manager/src/libembed-amd.ts#L33 const unpgkUrl = 'https://unpkg.com/'; const jsdelivrUrl = 'https://cdn.jsdelivr.net/npm/'; + +// tslint:disable: no-var-requires no-require-imports +const sanitize = require('sanitize-filename'); + function moduleNameToCDNUrl(cdn: string, moduleName: string, moduleVersion: string) { let packageName = moduleName; let fileName = 'index'; // default filename @@ -29,6 +38,16 @@ function moduleNameToCDNUrl(cdn: string, moduleName: string, moduleVersion: stri fileName = moduleName.substr(index + 1); packageName = moduleName.substr(0, index); } + if (cdn === jsdelivrUrl) { + // Js Delivr doesn't support ^ in the version. It needs an exact version + if (moduleVersion.startsWith('^')) { + moduleVersion = moduleVersion.slice(1); + } + // Js Delivr also needs the .js file on the end. + if (!fileName.endsWith('.js')) { + fileName = fileName.concat('.js'); + } + } return `${cdn}${packageName}@${moduleVersion}/dist/${fileName}`; } @@ -52,40 +71,177 @@ export class CDNWidgetScriptSourceProvider implements IWidgetScriptSourceProvide const settings = this.configurationSettings.getSettings(undefined); return settings.datascience.widgetScriptSources; } - public static validUrls = new Map(); + private cache = new Map(); constructor( private readonly configurationSettings: IConfigurationService, - private readonly httpClient: IHttpClient + private readonly httpClient: IHttpClient, + private readonly localResourceUriConverter: ILocalResourceUriConverter, + private readonly fileSystem: IFileSystem ) {} public dispose() { - // Noop. + this.cache.clear(); } public async getWidgetScriptSource(moduleName: string, moduleVersion: string): Promise { - const cdns = [...this.cdnProviders]; - while (cdns.length) { - const cdn = cdns.shift(); - const cdnBaseUrl = getCDNPrefix(cdn); - if (!cdnBaseUrl || !cdn) { - continue; + // First see if we already have it downloaded. + const key = this.getModuleKey(moduleName, moduleVersion); + const diskPath = path.join(this.localResourceUriConverter.rootScriptFolder.fsPath, key, 'index.js'); + let cached = this.cache.get(key); + let tempFile: TemporaryFile | undefined; + + // Might be on disk, try there first. + if (!cached) { + if (diskPath && (await this.fileSystem.fileExists(diskPath))) { + const scriptUri = (await this.localResourceUriConverter.asWebviewUri(Uri.file(diskPath))).toString(); + cached = { moduleName, scriptUri, source: 'cdn' }; + this.cache.set(key, cached); } - const scriptUri = moduleNameToCDNUrl(cdnBaseUrl, moduleName, moduleVersion); - const exists = await this.getUrlForWidget(cdn, scriptUri); - if (exists) { - return { moduleName, scriptUri, source: 'cdn' }; + } + + // If still not found, download it. + if (!cached) { + try { + // Make sure the disk path directory exists. We'll be downloading it to there. + await this.fileSystem.createDirectory(path.dirname(diskPath)); + + // Then get the first one that returns. + tempFile = await this.downloadFastestCDN(moduleName, moduleVersion); + if (tempFile) { + // Need to copy from the temporary file to our real file (note: VSC filesystem fails to copy so just use straight file system) + await fs.copyFile(tempFile.filePath, diskPath); + + // Now we can generate the script URI so the local converter doesn't try to copy it. + const scriptUri = ( + await this.localResourceUriConverter.asWebviewUri(Uri.file(diskPath)) + ).toString(); + cached = { moduleName, scriptUri, source: 'cdn' }; + } else { + cached = { moduleName }; + } + } catch (exc) { + traceError('Error downloading from CDN: ', exc); + cached = { moduleName }; + } finally { + if (tempFile) { + tempFile.dispose(); + } + } + this.cache.set(key, cached); + } + + return cached; + } + + private async downloadFastestCDN(moduleName: string, moduleVersion: string) { + const deferred = createDeferred(); + Promise.all( + // For each CDN, try to download it. + this.cdnProviders.map((cdn) => + this.downloadFromCDN(moduleName, moduleVersion, cdn).then((t) => { + // First one to get here wins. Meaning the first one that + // returns a valid temporary file. If a request doesn't download it will + // return undefined. + if (!deferred.resolved && t) { + deferred.resolve(t); + } + }) + ) + ) + .then((_a) => { + // If after running all requests, we're still not resolved, then return empty. + // This would happen if both unpkg.com and jsdelivr failed. + if (!deferred.resolved) { + deferred.resolve(undefined); + } + }) + .ignoreErrors(); + + // Note, we only wait until one download finishes. We don't need to wait + // for everybody (hence the use of the deferred) + return deferred.promise; + } + + private async downloadFromCDN( + moduleName: string, + moduleVersion: string, + cdn: WidgetCDNs + ): Promise { + // First validate CDN + const downloadUrl = await this.generateDownloadUri(moduleName, moduleVersion, cdn); + if (downloadUrl) { + // Then see if we can download the file. + try { + return await this.downloadFile(downloadUrl); + } catch (exc) { + // Something goes wrong, just fail } } - traceWarning(`Widget Script not found for ${moduleName}@${moduleVersion}`); - return { moduleName }; } - private async getUrlForWidget(cdn: string, url: string): Promise { - if (CDNWidgetScriptSourceProvider.validUrls.has(url)) { - return CDNWidgetScriptSourceProvider.validUrls.get(url)!; + + private async generateDownloadUri( + moduleName: string, + moduleVersion: string, + cdn: WidgetCDNs + ): Promise { + const cdnBaseUrl = getCDNPrefix(cdn); + if (cdnBaseUrl) { + return moduleNameToCDNUrl(cdnBaseUrl, moduleName, moduleVersion); } + return undefined; + } + + private getModuleKey(moduleName: string, moduleVersion: string) { + return sanitize(sha256().update(`${moduleName}${moduleVersion}`).digest('hex')); + } - const stopWatch = new StopWatch(); - const exists = await this.httpClient.exists(url); - sendTelemetryEvent(Telemetry.DiscoverIPyWidgetNamesCDNPerf, stopWatch.elapsedTime, { cdn, exists }); - CDNWidgetScriptSourceProvider.validUrls.set(url, exists); - return exists; + private handleResponse(req: request.Request, filePath: string): Promise { + const deferred = createDeferred(); + // tslint:disable-next-line: no-any + const errorHandler = (e: any) => { + traceError('Error downloading from CDN', e); + deferred.resolve(false); + }; + req.on('response', (r) => { + if (r.statusCode === 200) { + const ws = this.fileSystem.createWriteStream(filePath); + r.on('error', errorHandler) + .pipe(ws) + .on('close', () => deferred.resolve(true)); + } else if (r.statusCode === 429) { + // Special case busy. Sleep for 500 milliseconds + sleep(500) + .then(() => deferred.resolve(false)) + .ignoreErrors(); + } else { + deferred.resolve(false); + } + }).on('error', errorHandler); + return deferred.promise; + } + + private async downloadFile(downloadUrl: string): Promise { + // Create a temp file to download the results to + const tempFile = await this.fileSystem.createTemporaryFile('.js'); + + // Otherwise do an http get on the url. Retry at least 5 times + let retryCount = 5; + let success = false; + while (retryCount > 0 && !success) { + let req: request.Request; + try { + req = await this.httpClient.downloadFile(downloadUrl); + success = await this.handleResponse(req, tempFile.filePath); + } catch (exc) { + traceInfo(`Error downloading from ${downloadUrl}: `, exc); + } finally { + retryCount -= 1; + } + } + + // Once we make it out, return result + if (success) { + return tempFile; + } else { + tempFile.dispose(); + } } } diff --git a/src/client/datascience/ipywidgets/ipyWidgetMessageDispatcher.ts b/src/client/datascience/ipywidgets/ipyWidgetMessageDispatcher.ts index d97753b225a5..8e8c8f8a5c3e 100644 --- a/src/client/datascience/ipywidgets/ipyWidgetMessageDispatcher.ts +++ b/src/client/datascience/ipywidgets/ipyWidgetMessageDispatcher.ts @@ -14,7 +14,7 @@ import { createDeferred, Deferred } from '../../common/utils/async'; import { noop } from '../../common/utils/misc'; import { deserializeDataViews, serializeDataViews } from '../../common/utils/serializers'; import { sendTelemetryEvent } from '../../telemetry'; -import { Telemetry } from '../constants'; +import { Identifiers, Telemetry } from '../constants'; import { IInteractiveWindowMapping, IPyWidgetMessages } from '../interactive-common/interactiveWindowTypes'; import { INotebook, INotebookProvider, KernelSocketInformation } from '../types'; import { IIPyWidgetMessageDispatcher, IPyWidgetMessage } from './types'; @@ -280,9 +280,16 @@ export class IPyWidgetMessageDispatcher implements IIPyWidgetMessageDispatcher { return; } + traceInfo(`Registering commtarget ${targetName}`); this.commTargetsRegistered.add(targetName); this.pendingTargetNames.delete(targetName); - notebook.registerCommTarget(targetName, noop); + + // Skip the predefined target. It should have been registered + // inside the kernel on startup. However we + // still need to track it here. + if (targetName !== Identifiers.DefaultCommTarget) { + notebook.registerCommTarget(targetName, noop); + } } } diff --git a/src/client/datascience/ipywidgets/ipyWidgetScriptSource.ts b/src/client/datascience/ipywidgets/ipyWidgetScriptSource.ts index 0b3f888e6ad5..f0ece0610cd2 100644 --- a/src/client/datascience/ipywidgets/ipyWidgetScriptSource.ts +++ b/src/client/datascience/ipywidgets/ipyWidgetScriptSource.ts @@ -66,6 +66,7 @@ export class IPyWidgetScriptSource implements IInteractiveWindowListener, ILocal private pendingModuleRequests = new Map(); private readonly uriConversionPromises = new Map>(); private readonly targetWidgetScriptsFolder: string; + private readonly _rootScriptFolder: string; private readonly createTargetWidgetScriptsFolder: Promise; constructor( @inject(IDisposableRegistry) disposables: IDisposableRegistry, @@ -79,7 +80,8 @@ export class IPyWidgetScriptSource implements IInteractiveWindowListener, ILocal @inject(IPersistentStateFactory) private readonly stateFactory: IPersistentStateFactory, @inject(IExtensionContext) extensionContext: IExtensionContext ) { - this.targetWidgetScriptsFolder = path.join(extensionContext.extensionPath, 'tmp', 'nbextensions'); + this._rootScriptFolder = path.join(extensionContext.extensionPath, 'tmp', 'scripts'); + this.targetWidgetScriptsFolder = path.join(this._rootScriptFolder, 'nbextensions'); this.createTargetWidgetScriptsFolder = this.fs .directoryExists(this.targetWidgetScriptsFolder) .then(async (exists) => { @@ -108,30 +110,34 @@ export class IPyWidgetScriptSource implements IInteractiveWindowListener, ILocal * Copying into global workspace folder would also work, but over time this folder size could grow (in an unmanaged way). */ public async asWebviewUri(localResource: Uri): Promise { - if (this.notebookIdentity && !this.resourcesMappedToExtensionFolder.has(localResource.fsPath)) { - const deferred = createDeferred(); - this.resourcesMappedToExtensionFolder.set(localResource.fsPath, deferred.promise); - try { - // Create a file name such that it will be unique and consistent across VSC reloads. - // Only if original file has been modified should we create a new copy of the sam file. - const fileHash: string = await this.fs.getFileHash(localResource.fsPath); - const uniqueFileName = sanitize(sha256().update(`${localResource.fsPath}${fileHash}`).digest('hex')); - const targetFolder = await this.createTargetWidgetScriptsFolder; - const mappedResource = Uri.file( - path.join(targetFolder, `${uniqueFileName}${path.basename(localResource.fsPath)}`) - ); - if (!(await this.fs.fileExists(mappedResource.fsPath))) { - await this.fs.copyFile(localResource.fsPath, mappedResource.fsPath); + // Make a copy of the local file if not already in the correct location + if (!localResource.fsPath.startsWith(this._rootScriptFolder)) { + if (this.notebookIdentity && !this.resourcesMappedToExtensionFolder.has(localResource.fsPath)) { + const deferred = createDeferred(); + this.resourcesMappedToExtensionFolder.set(localResource.fsPath, deferred.promise); + try { + // Create a file name such that it will be unique and consistent across VSC reloads. + // Only if original file has been modified should we create a new copy of the sam file. + const fileHash: string = await this.fs.getFileHash(localResource.fsPath); + const uniqueFileName = sanitize( + sha256().update(`${localResource.fsPath}${fileHash}`).digest('hex') + ); + const targetFolder = await this.createTargetWidgetScriptsFolder; + const mappedResource = Uri.file( + path.join(targetFolder, `${uniqueFileName}${path.basename(localResource.fsPath)}`) + ); + if (!(await this.fs.fileExists(mappedResource.fsPath))) { + await this.fs.copyFile(localResource.fsPath, mappedResource.fsPath); + } + traceInfo(`Widget Script file ${localResource.fsPath} mapped to ${mappedResource.fsPath}`); + deferred.resolve(mappedResource); + } catch (ex) { + traceError(`Failed to map widget Script file ${localResource.fsPath}`); + deferred.reject(ex); } - traceInfo(`Widget Script file ${localResource.fsPath} mapped to ${mappedResource.fsPath}`); - deferred.resolve(mappedResource); - } catch (ex) { - traceError(`Failed to map widget Script file ${localResource.fsPath}`); - deferred.reject(ex); } + localResource = await this.resourcesMappedToExtensionFolder.get(localResource.fsPath)!; } - localResource = await this.resourcesMappedToExtensionFolder.get(localResource.fsPath)!; - const key = localResource.toString(); if (!this.uriConversionPromises.has(key)) { this.uriConversionPromises.set(key, createDeferred()); @@ -144,6 +150,10 @@ export class IPyWidgetScriptSource implements IInteractiveWindowListener, ILocal return this.uriConversionPromises.get(key)!.promise; } + public get rootScriptFolder(): Uri { + return Uri.file(this._rootScriptFolder); + } + public dispose() { while (this.disposables.length) { this.disposables.shift()?.dispose(); // NOSONAR diff --git a/src/client/datascience/ipywidgets/ipyWidgetScriptSourceProvider.ts b/src/client/datascience/ipywidgets/ipyWidgetScriptSourceProvider.ts index 61fcada1f0c1..d055095ebd34 100644 --- a/src/client/datascience/ipywidgets/ipyWidgetScriptSourceProvider.ts +++ b/src/client/datascience/ipywidgets/ipyWidgetScriptSourceProvider.ts @@ -161,7 +161,14 @@ export class IPyWidgetScriptSourceProvider implements IWidgetScriptSourceProvide // If we're allowed to use CDN providers, then use them, and use in order of preference. if (this.configuredScriptSources.length > 0) { - scriptProviders.push(new CDNWidgetScriptSourceProvider(this.configurationSettings, this.httpClient)); + scriptProviders.push( + new CDNWidgetScriptSourceProvider( + this.configurationSettings, + this.httpClient, + this.localResourceUriConverter, + this.fs + ) + ); } if (this.notebook.connection.localLaunch) { scriptProviders.push( diff --git a/src/client/datascience/ipywidgets/localWidgetScriptSourceProvider.ts b/src/client/datascience/ipywidgets/localWidgetScriptSourceProvider.ts index 4a4b4a1038e6..5b2ffcc92f8d 100644 --- a/src/client/datascience/ipywidgets/localWidgetScriptSourceProvider.ts +++ b/src/client/datascience/ipywidgets/localWidgetScriptSourceProvider.ts @@ -55,7 +55,7 @@ export class LocalWidgetScriptSourceProvider implements IWidgetScriptSourceProvi const validFiles = files.filter((file) => { // Should be of the form `/index.js` - const parts = file.split(path.sep); + const parts = file.split('/'); // On windows this uses the unix separator too. if (parts.length !== 2) { traceError('Incorrect file found when searching for nnbextension entrypoints'); return false; @@ -65,7 +65,7 @@ export class LocalWidgetScriptSourceProvider implements IWidgetScriptSourceProvi const mappedFiles = validFiles.map(async (file) => { // Should be of the form `/index.js` - const parts = file.split(path.sep); + const parts = file.split('/'); const moduleName = parts[0]; const fileUri = Uri.file(path.join(nbextensionsPath, file)); diff --git a/src/client/datascience/jupyter/jupyterSession.ts b/src/client/datascience/jupyter/jupyterSession.ts index e8c251572a0d..b8eb0fa5f83a 100644 --- a/src/client/datascience/jupyter/jupyterSession.ts +++ b/src/client/datascience/jupyter/jupyterSession.ts @@ -26,7 +26,7 @@ import { sleep, waitForPromise } from '../../common/utils/async'; import * as localize from '../../common/utils/localize'; import { noop } from '../../common/utils/misc'; import { captureTelemetry, sendTelemetryEvent } from '../../telemetry'; -import { Telemetry } from '../constants'; +import { Identifiers, Telemetry } from '../constants'; import { reportAction } from '../progress/decorator'; import { ReportableAction } from '../progress/types'; import { IConnection, IJupyterKernelSpec, IJupyterSession, KernelSocketInformation } from '../types'; @@ -505,6 +505,10 @@ export class JupyterSession implements IJupyterSession { // If we didn't make it out in ten seconds, indicate an error if (session.kernel && session.kernel.status === 'idle') { + // So that we don't have problems with ipywidgets, always register the default ipywidgets comm target. + // Restart sessions and retries might make this hard to do correctly otherwise. + session.kernel.registerCommTarget(Identifiers.DefaultCommTarget, noop); + return; } diff --git a/src/client/datascience/types.ts b/src/client/datascience/types.ts index 4fc71ebb8d10..aff13a24e863 100644 --- a/src/client/datascience/types.ts +++ b/src/client/datascience/types.ts @@ -375,6 +375,10 @@ export interface IDataScienceErrorHandler { * Given a local resource this will convert the Uri into a form such that it can be used in a WebView. */ export interface ILocalResourceUriConverter { + /** + * Root folder that scripts should be copied to. + */ + readonly rootScriptFolder: Uri; /** * Convert a uri for the local file system to one that can be used inside webviews. * diff --git a/src/datascience-ui/ipywidgets/container.tsx b/src/datascience-ui/ipywidgets/container.tsx index 85e85c732b58..96421537b448 100644 --- a/src/datascience-ui/ipywidgets/container.tsx +++ b/src/datascience-ui/ipywidgets/container.tsx @@ -193,7 +193,7 @@ export class WidgetManagerComponent extends React.Component { // Possible user has ignored some UI prompt and things are now in a state of limbo. // This way thigns will fall over sooner due to missing widget sources. const timeoutTime = this.timedoutWaitingForWidgetsToGetLoaded - ? 1_000 + ? 10_000 : this.loaderSettings.timeoutWaitingForScriptToLoad; setTimeout(() => { diff --git a/src/test/datascience/ipywidgets/cdnWidgetScriptSourceProvider.unit.test.ts b/src/test/datascience/ipywidgets/cdnWidgetScriptSourceProvider.unit.test.ts index 22f3faae9788..5c775de7ba69 100644 --- a/src/test/datascience/ipywidgets/cdnWidgetScriptSourceProvider.unit.test.ts +++ b/src/test/datascience/ipywidgets/cdnWidgetScriptSourceProvider.unit.test.ts @@ -1,38 +1,116 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. import { assert } from 'chai'; +import * as fs from 'fs-extra'; +import { sha256 } from 'hash.js'; +import { shutdown } from 'log4js'; +import * as nock from 'nock'; +import * as path from 'path'; +import { Readable } from 'stream'; import { anything, instance, mock, verify, when } from 'ts-mockito'; -import { EventEmitter } from 'vscode'; +import { EventEmitter, Uri } from 'vscode'; import { PythonSettings } from '../../../client/common/configSettings'; import { ConfigurationService } from '../../../client/common/configuration/service'; import { HttpClient } from '../../../client/common/net/httpClient'; +import { FileSystem } from '../../../client/common/platform/fileSystem'; +import { IFileSystem } from '../../../client/common/platform/types'; import { IConfigurationService, IHttpClient, WidgetCDNs } from '../../../client/common/types'; import { noop } from '../../../client/common/utils/misc'; +import { EXTENSION_ROOT_DIR } from '../../../client/constants'; import { CDNWidgetScriptSourceProvider } from '../../../client/datascience/ipywidgets/cdnWidgetScriptSourceProvider'; -import { IWidgetScriptSourceProvider, WidgetScriptSource } from '../../../client/datascience/ipywidgets/types'; +import { IPyWidgetScriptSource } from '../../../client/datascience/ipywidgets/ipyWidgetScriptSource'; +import { IWidgetScriptSourceProvider } from '../../../client/datascience/ipywidgets/types'; import { JupyterNotebookBase } from '../../../client/datascience/jupyter/jupyterNotebook'; -import { IConnection, INotebook } from '../../../client/datascience/types'; +import { IConnection, ILocalResourceUriConverter, INotebook } from '../../../client/datascience/types'; + +// tslint:disable: no-var-requires no-require-imports max-func-body-length no-any match-default-export-name +const request = require('request'); +const sanitize = require('sanitize-filename'); const unpgkUrl = 'https://unpkg.com/'; const jsdelivrUrl = 'https://cdn.jsdelivr.net/npm/'; // tslint:disable: max-func-body-length no-any -suite('Data Science - ipywidget - CDN', () => { +suite('DataScience - ipywidget - CDN', () => { let scriptSourceProvider: IWidgetScriptSourceProvider; let notebook: INotebook; let configService: IConfigurationService; let httpClient: IHttpClient; let settings: PythonSettings; + let fileSystem: IFileSystem; + let webviewUriConverter: ILocalResourceUriConverter; + let tempFileCount = 0; setup(() => { notebook = mock(JupyterNotebookBase); configService = mock(ConfigurationService); httpClient = mock(HttpClient); + fileSystem = mock(FileSystem); + webviewUriConverter = mock(IPyWidgetScriptSource); settings = { datascience: { widgetScriptSources: [] } } as any; when(configService.getSettings(anything())).thenReturn(settings as any); - CDNWidgetScriptSourceProvider.validUrls = new Map(); - scriptSourceProvider = new CDNWidgetScriptSourceProvider(instance(configService), instance(httpClient)); + when(httpClient.downloadFile(anything())).thenCall(request); + when(fileSystem.fileExists(anything())).thenCall((f) => fs.pathExists(f)); + + when(fileSystem.createTemporaryFile(anything())).thenCall(createTemporaryFile); + when(fileSystem.createWriteStream(anything())).thenCall((p) => fs.createWriteStream(p)); + when(fileSystem.createDirectory(anything())).thenCall((d) => fs.ensureDir(d)); + when(webviewUriConverter.rootScriptFolder).thenReturn( + Uri.file(path.join(EXTENSION_ROOT_DIR, 'tmp', 'scripts')) + ); + when(webviewUriConverter.asWebviewUri(anything())).thenCall((u) => u); + scriptSourceProvider = new CDNWidgetScriptSourceProvider( + instance(configService), + instance(httpClient), + instance(webviewUriConverter), + instance(fileSystem) + ); + }); + + shutdown(() => { + clearDiskCache(); }); + function createStreamFromString(str: string) { + const readable = new Readable(); + readable._read = noop; + readable.push(str); + readable.push(null); + return readable; + } + + function createTemporaryFile(ext: string) { + tempFileCount += 1; + + // Put temp files next to extension so we can clean them up later + const filePath = path.join( + EXTENSION_ROOT_DIR, + 'tmp', + 'tempfile_loc', + `tempfile_for_test${tempFileCount}.${ext}` + ); + fs.createFileSync(filePath); + return { + filePath, + dispose: () => { + fs.removeSync(filePath); + } + }; + } + + function generateScriptName(moduleName: string, moduleVersion: string) { + const hash = sanitize(sha256().update(`${moduleName}${moduleVersion}`).digest('hex')); + return Uri.file(path.join(EXTENSION_ROOT_DIR, 'tmp', 'scripts', hash, 'index.js')).toString(); + } + + function clearDiskCache() { + try { + fs.removeSync(path.join(EXTENSION_ROOT_DIR, 'tmp', 'scripts')); + fs.removeSync(path.join(EXTENSION_ROOT_DIR, 'tmp', 'tempfile_loc')); + } catch { + // Swallow any errors here + } + } + [true, false].forEach((localLaunch) => { suite(localLaunch ? 'Local Jupyter Server' : 'Remote Jupyter Server', () => { setup(() => { @@ -61,123 +139,129 @@ suite('Data Science - ipywidget - CDN', () => { suite(cdn, () => { const moduleName = 'HelloWorld'; const moduleVersion = '1'; - let expectedSource = ''; + let baseUrl = ''; + let getUrl = ''; + let scriptUri = ''; setup(() => { - const baseUrl = cdn === 'unpkg.com' ? unpgkUrl : jsdelivrUrl; - expectedSource = `${baseUrl}${moduleName}@${moduleVersion}/dist/index`; - CDNWidgetScriptSourceProvider.validUrls = new Map(); + baseUrl = cdn === 'unpkg.com' ? unpgkUrl : jsdelivrUrl; + getUrl = + cdn === 'unpkg.com' + ? `${moduleName}@${moduleVersion}/dist/index` + : `${moduleName}@${moduleVersion}/dist/index.js`; + scriptUri = generateScriptName(moduleName, moduleVersion); + }); + teardown(() => { + clearDiskCache(); + scriptSourceProvider.dispose(); + nock.cleanAll(); }); - test('Get widget source from CDN', async () => { + test('Ensure widget script is downloaded once and cached', async () => { updateCDNSettings(cdn); - when(httpClient.exists(anything())).thenResolve(true); + let downloadCount = 0; + nock(baseUrl) + .get(`/${getUrl}`) + .reply(200, () => { + downloadCount += 1; + return createStreamFromString('foo'); + }); const value = await scriptSourceProvider.getWidgetScriptSource(moduleName, moduleVersion); assert.deepEqual(value, { moduleName: 'HelloWorld', - scriptUri: expectedSource, + scriptUri, source: 'cdn' }); - verify(httpClient.exists(anything())).once(); - }); - test('Ensure widgtet script is downloaded once and cached', async () => { - updateCDNSettings(cdn); - when(httpClient.exists(anything())).thenResolve(true); - const expectedValue: WidgetScriptSource = { - moduleName: 'HelloWorld', - scriptUri: expectedSource, - source: 'cdn' - }; - const value = await scriptSourceProvider.getWidgetScriptSource(moduleName, moduleVersion); - assert.deepEqual(value, expectedValue); - const value1 = await scriptSourceProvider.getWidgetScriptSource(moduleName, moduleVersion); - assert.deepEqual(value1, expectedValue); const value2 = await scriptSourceProvider.getWidgetScriptSource(moduleName, moduleVersion); - assert.deepEqual(value2, expectedValue); - // Only one http request - verify(httpClient.exists(anything())).once(); + assert.deepEqual(value2, { + moduleName: 'HelloWorld', + scriptUri, + source: 'cdn' + }); + + assert.equal(downloadCount, 1, 'Downloaded more than once'); }); test('No script source if package does not exist on CDN', async () => { updateCDNSettings(cdn); - when(httpClient.exists(anything())).thenResolve(false); - - const value = await scriptSourceProvider.getWidgetScriptSource(moduleName, moduleVersion); - - assert.deepEqual(value, { moduleName: 'HelloWorld' }); - verify(httpClient.exists(anything())).once(); - }); - test('No script source if package does not exist on both CDNs', async () => { - updateCDNSettings('jsdelivr.com', 'unpkg.com'); - when(httpClient.exists(anything())).thenResolve(false); - - const value = await scriptSourceProvider.getWidgetScriptSource(moduleName, moduleVersion); - - assert.deepEqual(value, { moduleName: 'HelloWorld' }); - }); - test('Give preference to jsdelivr over unpkg', async () => { - updateCDNSettings('jsdelivr.com', 'unpkg.com'); - when(httpClient.exists(anything())).thenResolve(true); + nock(baseUrl).get(`/${getUrl}`).replyWithError('404'); const value = await scriptSourceProvider.getWidgetScriptSource(moduleName, moduleVersion); assert.deepEqual(value, { - moduleName: 'HelloWorld', - scriptUri: `${jsdelivrUrl}${moduleName}@${moduleVersion}/dist/index`, - source: 'cdn' + moduleName: 'HelloWorld' }); - verify(httpClient.exists(anything())).once(); }); - test('Give preference to unpkg over jsdelivr', async () => { - updateCDNSettings('unpkg.com', 'jsdelivr.com'); - when(httpClient.exists(anything())).thenResolve(true); - + test('Script source if package does not exist on both CDNs', async () => { + // Add the other cdn (the opposite of the working one) + const cdns = + cdn === 'unpkg.com' + ? ([cdn, 'jsdelivr.com'] as WidgetCDNs[]) + : ([cdn, 'unpkg.com'] as WidgetCDNs[]); + updateCDNSettings(cdns[0], cdns[1]); + // Make only one cdn available + when(httpClient.exists(anything())).thenCall((a) => { + if (a.includes(cdn[0])) { + return true; + } + return false; + }); + nock(baseUrl) + .get(`/${getUrl}`) + .reply(200, () => { + return createStreamFromString('foo'); + }); const value = await scriptSourceProvider.getWidgetScriptSource(moduleName, moduleVersion); assert.deepEqual(value, { moduleName: 'HelloWorld', - scriptUri: `${unpgkUrl}${moduleName}@${moduleVersion}/dist/index`, + scriptUri, source: 'cdn' }); - verify(httpClient.exists(anything())).once(); }); - test('Get Script from unpk if jsdelivr fails', async () => { - updateCDNSettings('jsdelivr.com', 'unpkg.com'); - when(httpClient.exists(anything())).thenCall( - async (url: string) => !url.startsWith(jsdelivrUrl) - ); + test('Retry if busy', async () => { + let retryCount = 0; + updateCDNSettings(cdn); + when(httpClient.exists(anything())).thenResolve(true); + nock(baseUrl).get(`/${getUrl}`).twice().replyWithError('Not found'); + nock(baseUrl) + .get(`/${getUrl}`) + .thrice() + .reply(200, () => { + retryCount = 3; + return createStreamFromString('foo'); + }); + + // Then see if we can get it still. const value = await scriptSourceProvider.getWidgetScriptSource(moduleName, moduleVersion); assert.deepEqual(value, { moduleName: 'HelloWorld', - scriptUri: `${unpgkUrl}${moduleName}@${moduleVersion}/dist/index`, + scriptUri, source: 'cdn' }); - verify(httpClient.exists(anything())).twice(); + assert.equal(retryCount, 3, 'Did not actually retry'); }); - test('Get Script from jsdelivr if unpkg fails', async () => { - updateCDNSettings('unpkg.com', 'jsdelivr.com'); - when(httpClient.exists(anything())).thenCall(async (url: string) => !url.startsWith(unpgkUrl)); + test('Script source already on disk', async () => { + updateCDNSettings(cdn); + // Make nobody available + when(httpClient.exists(anything())).thenResolve(true); + // Write to where the file should eventually end up + const filePath = Uri.parse(scriptUri).fsPath; + await fs.createFile(filePath); + await fs.writeFile(filePath, 'foo'); + + // Then see if we can get it still. const value = await scriptSourceProvider.getWidgetScriptSource(moduleName, moduleVersion); assert.deepEqual(value, { moduleName: 'HelloWorld', - scriptUri: `${jsdelivrUrl}${moduleName}@${moduleVersion}/dist/index`, + scriptUri, source: 'cdn' }); - verify(httpClient.exists(anything())).twice(); - }); - test('No script source if downloading from both CDNs fail', async () => { - updateCDNSettings('unpkg.com', 'jsdelivr.com'); - when(httpClient.exists(anything())).thenResolve(false); - - const value = await scriptSourceProvider.getWidgetScriptSource(moduleName, moduleVersion); - - assert.deepEqual(value, { moduleName: 'HelloWorld' }); - verify(httpClient.exists(anything())).twice(); }); }); }); diff --git a/src/test/datascience/ipywidgets/ipyWidgetScriptSourceProvider.unit.test.ts b/src/test/datascience/ipywidgets/ipyWidgetScriptSourceProvider.unit.test.ts index 8c79ca09fe98..3b2ba1d3c536 100644 --- a/src/test/datascience/ipywidgets/ipyWidgetScriptSourceProvider.unit.test.ts +++ b/src/test/datascience/ipywidgets/ipyWidgetScriptSourceProvider.unit.test.ts @@ -54,7 +54,6 @@ suite('xxxData Science - ipywidget - Widget Script Source Provider', () => { when(configService.getSettings(anything())).thenReturn(settings as any); when(userSelectedOkOrDoNotShowAgainInPrompt.value).thenReturn(false); when(userSelectedOkOrDoNotShowAgainInPrompt.updateValue(anything())).thenResolve(); - CDNWidgetScriptSourceProvider.validUrls = new Map(); scriptSourceProvider = new IPyWidgetScriptSourceProvider( instance(notebook), instance(resourceConverter), diff --git a/src/test/datascience/jupyter/jupyterSession.unit.test.ts b/src/test/datascience/jupyter/jupyterSession.unit.test.ts index a624fe232809..a98d2ec2b7cc 100644 --- a/src/test/datascience/jupyter/jupyterSession.unit.test.ts +++ b/src/test/datascience/jupyter/jupyterSession.unit.test.ts @@ -208,7 +208,8 @@ suite('Data Science - JupyterSession', () => { }, kernel: { status: 'idle', - restart: () => (restartCount = restartCount + 1) + restart: () => (restartCount = restartCount + 1), + registerCommTarget: noop }, shutdown: () => Promise.resolve(), isRemoteSession: false @@ -225,6 +226,7 @@ suite('Data Science - JupyterSession', () => { remoteSessionInstance = instance(remoteSession); remoteSessionInstance.isRemoteSession = false; when(remoteSession.kernel).thenReturn(instance(remoteKernel)); + when(remoteKernel.registerCommTarget(anything(), anything())).thenReturn(); when(sessionManager.startNew(anything())).thenCall(() => { return Promise.resolve(instance(remoteSession)); }); From d6212a3d45ca19997db60c15e923765e918f8706 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Wed, 22 Apr 2020 11:26:25 -0700 Subject: [PATCH 2/3] Fix container timeout from firing unnecessarily (#11336) * Fix container timeout from firing unnecessarily * Check for undefined for timer --- news/2 Fixes/11334.md | 1 + src/datascience-ui/ipywidgets/container.tsx | 58 ++++++++++++++------- 2 files changed, 40 insertions(+), 19 deletions(-) create mode 100644 news/2 Fixes/11334.md diff --git a/news/2 Fixes/11334.md b/news/2 Fixes/11334.md new file mode 100644 index 000000000000..cb594f2fca6f --- /dev/null +++ b/news/2 Fixes/11334.md @@ -0,0 +1 @@ +Error: Timeout is shown after running any widget more than once. \ No newline at end of file diff --git a/src/datascience-ui/ipywidgets/container.tsx b/src/datascience-ui/ipywidgets/container.tsx index 96421537b448..1072ccf33849 100644 --- a/src/datascience-ui/ipywidgets/container.tsx +++ b/src/datascience-ui/ipywidgets/container.tsx @@ -34,7 +34,10 @@ type Props = { export class WidgetManagerComponent extends React.Component { private readonly widgetManager: WidgetManager; - private readonly widgetSourceRequests = new Map>(); + private readonly widgetSourceRequests = new Map< + string, + { deferred: Deferred; timer: NodeJS.Timeout | number | undefined } + >(); private timedoutWaitingForWidgetsToGetLoaded?: boolean; private widgetsCanLoadFromCDN: boolean = false; private readonly loaderSettings = { @@ -107,12 +110,19 @@ export class WidgetManagerComponent extends React.Component { sources.forEach((source) => { // We have fetched the script sources for all of these modules. // In some cases we might not have the source, meaning we don't have it or couldn't find it. - let deferred = this.widgetSourceRequests.get(source.moduleName); - if (!deferred) { - deferred = createDeferred(); - this.widgetSourceRequests.set(source.moduleName, deferred); + let request = this.widgetSourceRequests.get(source.moduleName); + if (!request) { + request = { + deferred: createDeferred(), + timer: undefined + }; + this.widgetSourceRequests.set(source.moduleName, request); + } + request.deferred.resolve(); + if (request.timer !== undefined) { + // tslint:disable-next-line: no-any + clearTimeout(request.timer as any); // This is to make this work on Node and Browser } - deferred.resolve(); }); } private registerScriptSourceInRequirejs(source?: WidgetScriptSource) { @@ -180,10 +190,12 @@ export class WidgetManagerComponent extends React.Component { private loadWidgetScript(moduleName: string, moduleVersion: string): Promise { // tslint:disable-next-line: no-console console.log(`Fetch IPyWidget source for ${moduleName}`); - let deferred = this.widgetSourceRequests.get(moduleName); - if (!deferred) { - deferred = createDeferred(); - this.widgetSourceRequests.set(moduleName, deferred); + let request = this.widgetSourceRequests.get(moduleName); + if (!request) { + request = { + deferred: createDeferred(), + timer: undefined + }; // If we timeout, then resolve this promise. // We don't want the calling code to unnecessary wait for too long. @@ -193,18 +205,26 @@ export class WidgetManagerComponent extends React.Component { // Possible user has ignored some UI prompt and things are now in a state of limbo. // This way thigns will fall over sooner due to missing widget sources. const timeoutTime = this.timedoutWaitingForWidgetsToGetLoaded - ? 10_000 + ? 5_000 : this.loaderSettings.timeoutWaitingForScriptToLoad; - setTimeout(() => { - // tslint:disable-next-line: no-console - console.error(`Timeout waiting to get widget source for ${moduleName}, ${moduleVersion}`); - this.handleLoadError('', moduleName, moduleVersion, new Error('Timeout'), true).ignoreErrors(); - if (deferred) { - deferred.resolve(); + request.timer = setTimeout(() => { + if (request && !request.deferred.resolved) { + // tslint:disable-next-line: no-console + console.error(`Timeout waiting to get widget source for ${moduleName}, ${moduleVersion}`); + this.handleLoadError( + '', + moduleName, + moduleVersion, + new Error(`Timeout getting source for ${moduleName}:${moduleVersion}`), + true + ).ignoreErrors(); + request.deferred.resolve(); + this.timedoutWaitingForWidgetsToGetLoaded = true; } - this.timedoutWaitingForWidgetsToGetLoaded = true; }, timeoutTime); + + this.widgetSourceRequests.set(moduleName, request); } // Whether we have the scripts or not, send message to extension. // Useful telemetry and also we know it was explicity requestd by ipywidgest. @@ -213,7 +233,7 @@ export class WidgetManagerComponent extends React.Component { { moduleName, moduleVersion } ); - return deferred.promise.catch((ex) => + return request.deferred.promise.catch((ex) => // tslint:disable-next-line: no-console console.error(`Failed to load Widget Script from Extension for for ${moduleName}, ${moduleVersion}`, ex) ); From f45978652712d1c1679b8870fd8c42f1900d8c95 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Wed, 22 Apr 2020 11:30:21 -0700 Subject: [PATCH 3/3] Update changelog --- CHANGELOG.md | 63 +++++++++++++++++++++++++++++++++++++++++++ news/2 Fixes/11274.md | 1 - news/2 Fixes/11334.md | 1 - 3 files changed, 63 insertions(+), 2 deletions(-) delete mode 100644 news/2 Fixes/11274.md delete mode 100644 news/2 Fixes/11334.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 3f6d02f254dc..9e6d9574d8b6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,68 @@ # Changelog +## 2020.4.1 (27 April 2020) + +### Fixes + +1. Fix issue where downloading ipywidgets from the CDN might be busy. + ([#11274](https://github.com/Microsoft/vscode-python/issues/11274)) +1. Error: Timeout is shown after running any widget more than once. + ([#11334](https://github.com/Microsoft/vscode-python/issues/11334)) + +### Thanks + +Thanks to the following projects which we fully rely on to provide some of +our features: + +- [debugpy](https://pypi.org/project/debugpy/) +- [isort](https://pypi.org/project/isort/) +- [jedi](https://pypi.org/project/jedi/) + and [parso](https://pypi.org/project/parso/) +- [Microsoft Python Language Server](https://github.com/microsoft/python-language-server) +- [ptvsd](https://pypi.org/project/ptvsd/) +- [exuberant ctags](http://ctags.sourceforge.net/) (user-installed) +- [rope](https://pypi.org/project/rope/) (user-installed) + +Also thanks to the various projects we provide integrations with which help +make this extension useful: + +- Debugging support: + [Django](https://pypi.org/project/Django/), + [Flask](https://pypi.org/project/Flask/), + [gevent](https://pypi.org/project/gevent/), + [Jinja](https://pypi.org/project/Jinja/), + [Pyramid](https://pypi.org/project/pyramid/), + [PySpark](https://pypi.org/project/pyspark/), + [Scrapy](https://pypi.org/project/Scrapy/), + [Watson](https://pypi.org/project/Watson/) +- Formatting: + [autopep8](https://pypi.org/project/autopep8/), + [black](https://pypi.org/project/black/), + [yapf](https://pypi.org/project/yapf/) +- Interpreter support: + [conda](https://conda.io/), + [direnv](https://direnv.net/), + [pipenv](https://pypi.org/project/pipenv/), + [pyenv](https://github.com/pyenv/pyenv), + [venv](https://docs.python.org/3/library/venv.html#module-venv), + [virtualenv](https://pypi.org/project/virtualenv/) +- Linting: + [bandit](https://pypi.org/project/bandit/), + [flake8](https://pypi.org/project/flake8/), + [mypy](https://pypi.org/project/mypy/), + [prospector](https://pypi.org/project/prospector/), + [pylint](https://pypi.org/project/pylint/), + [pydocstyle](https://pypi.org/project/pydocstyle/), + [pylama](https://pypi.org/project/pylama/) +- Testing: + [nose](https://pypi.org/project/nose/), + [pytest](https://pypi.org/project/pytest/), + [unittest](https://docs.python.org/3/library/unittest.html#module-unittest) + +And finally thanks to the [Python](https://www.python.org/) development team and +community for creating a fantastic programming language and community to be a +part of! + ## 2020.4.0 (20 April 2020) ### Enhancements diff --git a/news/2 Fixes/11274.md b/news/2 Fixes/11274.md deleted file mode 100644 index d179a5a5592e..000000000000 --- a/news/2 Fixes/11274.md +++ /dev/null @@ -1 +0,0 @@ -Fix issue where downloading ipywidgets from the CDN might be busy. \ No newline at end of file diff --git a/news/2 Fixes/11334.md b/news/2 Fixes/11334.md deleted file mode 100644 index cb594f2fca6f..000000000000 --- a/news/2 Fixes/11334.md +++ /dev/null @@ -1 +0,0 @@ -Error: Timeout is shown after running any widget more than once. \ No newline at end of file