From 43000bb9f6de730b710d89c3e64eedaf0317025a Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Mon, 20 Apr 2020 16:38:10 -0700 Subject: [PATCH 01/12] Working downloader --- src/client/datascience/constants.ts | 1 + .../cdnWidgetScriptSourceProvider.ts | 212 ++++++++++++++++-- .../ipywidgets/ipyWidgetMessageDispatcher.ts | 11 +- .../ipywidgets/ipyWidgetScriptSource.ts | 54 +++-- .../ipyWidgetScriptSourceProvider.ts | 9 +- .../datascience/jupyter/jupyterSession.ts | 6 +- src/client/datascience/types.ts | 4 + src/datascience-ui/ipywidgets/container.tsx | 2 +- ...cdnWidgetScriptSourceProvider.unit.test.ts | 167 +++++++------- 9 files changed, 325 insertions(+), 141 deletions(-) diff --git a/src/client/datascience/constants.ts b/src/client/datascience/constants.ts index a0abc5fefeab..d16dfd49b0de 100644 --- a/src/client/datascience/constants.ts +++ b/src/client/datascience/constants.ts @@ -373,6 +373,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..0ca293830ae5 100644 --- a/src/client/datascience/ipywidgets/cdnWidgetScriptSourceProvider.ts +++ b/src/client/datascience/ipywidgets/cdnWidgetScriptSourceProvider.ts @@ -3,16 +3,28 @@ '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 { createDeferred, sleep } from '../../common/utils/async'; import { StopWatch } from '../../common/utils/stopWatch'; import { sendTelemetryEvent } from '../../telemetry'; import { Telemetry } from '../constants'; +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 +41,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}`; } @@ -53,39 +75,185 @@ export class CDNWidgetScriptSourceProvider implements IWidgetScriptSourceProvide 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. } 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); + + // 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) { + // 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. + const file = await this.downloadFastestCDN(moduleName, moduleVersion); + try { + if (file) { + // 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(file.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 (file) { + file.dispose(); + } } + this.cache.set(key, cached); } - traceWarning(`Widget Script not found for ${moduleName}@${moduleVersion}`); - return { moduleName }; + + 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. + if (!deferred.resolved && t) { + deferred.resolve(t); + } + }) + ) + ) + .then((_a) => { + // If still not resolved, then return empty + if (!deferred.resolved) { + deferred.resolve(undefined); + } + }) + .ignoreErrors(); + return deferred.promise; } - private async getUrlForWidget(cdn: string, url: string): Promise { - if (CDNWidgetScriptSourceProvider.validUrls.has(url)) { - return CDNWidgetScriptSourceProvider.validUrls.get(url)!; + + 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 + } } + } - 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 async generateDownloadUri( + moduleName: string, + moduleVersion: string, + cdn: WidgetCDNs + ): Promise { + const cdnBaseUrl = getCDNPrefix(cdn); + if (cdnBaseUrl) { + // May have already been validated. + const url = moduleNameToCDNUrl(cdnBaseUrl, moduleName, moduleVersion); + if (CDNWidgetScriptSourceProvider.validUrls.has(url)) { + return url; + } + + // Try pinging first. + 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 ? url : undefined; + } + return undefined; + } + + private getModuleKey(moduleName: string, moduleVersion: string) { + return sanitize(sha256().update(`${moduleName}${moduleVersion}`).digest('hex')); + } + + private async downloadFile(downloadUrl: string | undefined): Promise { + // Download URL should not be undefined (we validated in a filter above) + if (!downloadUrl) { + throw new Error('Cannot download from an undefined CDN'); + } + + // 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); + } catch (exc) { + traceInfo(`Error downloading from ${downloadUrl}: `, exc); + retryCount -= 1; + + // CDN may be busy, give it a break. + await sleep(500); + continue; + } + + try { + if (req) { + // Write to our temp file. + const tempWriteStream = this.fileSystem.createWriteStream(tempFile.filePath); + const deferred = createDeferred(); + req.on('error', (e) => { + traceError(`Error downloading from CDN: `, e); + deferred.reject(e); + }) + .pipe(tempWriteStream) + .on('close', () => { + deferred.resolve(); + }); + await deferred.promise; + success = true; + } + } catch (exc) { + // Don't do anything if we fail to write it. + traceError(`Error writing CDN download to disk: `, exc); + break; + } + } + + // 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..4ce415714c5a 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 rootScriptsFolder: 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.rootScriptsFolder = path.join(extensionContext.extensionPath, 'tmp', 'scripts'); + this.targetWidgetScriptsFolder = path.join(this.rootScriptsFolder, '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.rootScriptsFolder)) { + 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.rootScriptsFolder); + } + 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 f74d5fc0aba7..0a5385143df8 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 && this.notebook.connection.localLaunch) { scriptProviders.push( diff --git a/src/client/datascience/jupyter/jupyterSession.ts b/src/client/datascience/jupyter/jupyterSession.ts index c738523b6b7e..3dceef14b58b 100644 --- a/src/client/datascience/jupyter/jupyterSession.ts +++ b/src/client/datascience/jupyter/jupyterSession.ts @@ -23,7 +23,7 @@ import * as localize from '../../common/utils/localize'; import { noop } from '../../common/utils/misc'; import { captureTelemetry } from '../../telemetry'; import { BaseJupyterSession, ISession, JupyterSessionStartError } from '../baseJupyterSession'; -import { Telemetry } from '../constants'; +import { Identifiers, Telemetry } from '../constants'; import { reportAction } from '../progress/decorator'; import { ReportableAction } from '../progress/types'; import { IJupyterConnection, IJupyterKernelSpec } from '../types'; @@ -253,6 +253,10 @@ export class JupyterSession extends BaseJupyterSession { // 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 e9935396435b..8d04493344d0 100644 --- a/src/client/datascience/types.ts +++ b/src/client/datascience/types.ts @@ -435,6 +435,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 4b8780349c47..f72ad22c5a8e 100644 --- a/src/datascience-ui/ipywidgets/container.tsx +++ b/src/datascience-ui/ipywidgets/container.tsx @@ -182,7 +182,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 1e8834d54537..df6a3d7377d3 100644 --- a/src/test/datascience/ipywidgets/cdnWidgetScriptSourceProvider.unit.test.ts +++ b/src/test/datascience/ipywidgets/cdnWidgetScriptSourceProvider.unit.test.ts @@ -1,38 +1,84 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. import { assert } from 'chai'; +import * as fs from 'fs-extra'; +import * as nock from 'nock'; +import * as os from 'os'; +import * as path from 'path'; +import { Readable } from 'stream'; import { anything, instance, mock, verify, when } from 'ts-mockito'; import { EventEmitter } 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 { CDNWidgetScriptSourceProvider } from '../../../client/datascience/ipywidgets/cdnWidgetScriptSourceProvider'; +import { IPyWidgetScriptSource } from '../../../client/datascience/ipywidgets/ipyWidgetScriptSource'; import { IWidgetScriptSourceProvider, WidgetScriptSource } from '../../../client/datascience/ipywidgets/types'; import { JupyterNotebookBase } from '../../../client/datascience/jupyter/jupyterNotebook'; -import { IJupyterConnection, INotebook } from '../../../client/datascience/types'; +import { IJupyterConnection, 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 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); + 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)); CDNWidgetScriptSourceProvider.validUrls = new Map(); - scriptSourceProvider = new CDNWidgetScriptSourceProvider(instance(configService), instance(httpClient)); + scriptSourceProvider = new CDNWidgetScriptSourceProvider( + instance(configService), + instance(httpClient), + instance(webviewUriConverter), + instance(fileSystem) + ); }); + 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; + const filePath = path.join(os.tmpdir(), `tempfile_for_test${tempFileCount}.${ext}`); + return { + filePath, + dispose: () => { + fs.removeSync(filePath); + } + }; + } + [true, false].forEach((localLaunch) => { suite(localLaunch ? 'Local Jupyter Server' : 'Remote Jupyter Server', () => { setup(() => { @@ -65,14 +111,19 @@ suite('Data Science - ipywidget - CDN', () => { const moduleName = 'HelloWorld'; const moduleVersion = '1'; let expectedSource = ''; + let baseUrl = ''; + let getUrl = ''; setup(() => { - const baseUrl = cdn === 'unpkg.com' ? unpgkUrl : jsdelivrUrl; - expectedSource = `${baseUrl}${moduleName}@${moduleVersion}/dist/index`; + baseUrl = cdn === 'unpkg.com' ? unpgkUrl : jsdelivrUrl; + getUrl = + cdn === 'unpkg.com' + ? `${moduleName}@${moduleVersion}/dist/index` + : `${moduleName}@${moduleVersion}/dist/index.js`; + expectedSource = `${baseUrl}${getUrl}`; CDNWidgetScriptSourceProvider.validUrls = new Map(); }); test('Get widget source from CDN', async () => { updateCDNSettings(cdn); - when(httpClient.exists(anything())).thenResolve(true); const value = await scriptSourceProvider.getWidgetScriptSource(moduleName, moduleVersion); @@ -81,107 +132,39 @@ suite('Data Science - ipywidget - CDN', () => { scriptUri: expectedSource, 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(); - }); - 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); + test('Ensure widget script is downloaded once and cached', async () => { + 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: `${jsdelivrUrl}${moduleName}@${moduleVersion}/dist/index`, - source: 'cdn' - }); - verify(httpClient.exists(anything())).once(); - }); - test('Give preference to unpkg over jsdelivr', async () => { - updateCDNSettings('unpkg.com', 'jsdelivr.com'); - when(httpClient.exists(anything())).thenResolve(true); - - const value = await scriptSourceProvider.getWidgetScriptSource(moduleName, moduleVersion); - - assert.deepEqual(value, { - moduleName: 'HelloWorld', - scriptUri: `${unpgkUrl}${moduleName}@${moduleVersion}/dist/index`, - 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) - ); - - const value = await scriptSourceProvider.getWidgetScriptSource(moduleName, moduleVersion); - - assert.deepEqual(value, { - moduleName: 'HelloWorld', - scriptUri: `${unpgkUrl}${moduleName}@${moduleVersion}/dist/index`, + scriptUri: expectedSource, source: 'cdn' }); - verify(httpClient.exists(anything())).twice(); - }); - 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)); - const value = await scriptSourceProvider.getWidgetScriptSource(moduleName, moduleVersion); + const value2 = await scriptSourceProvider.getWidgetScriptSource(moduleName, moduleVersion); - assert.deepEqual(value, { + assert.deepEqual(value2, { moduleName: 'HelloWorld', - scriptUri: `${jsdelivrUrl}${moduleName}@${moduleVersion}/dist/index`, + scriptUri: expectedSource, 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(); + assert.equal(downloadCount, 1, 'Downloaded more than once'); }); + test('No script source if package does not exist on CDN', async () => {}); + test('No script source if package does not exist on both CDNs', async () => {}); + test('Get Script from unpk if jsdelivr fails', async () => {}); + test('Get Script from jsdelivr if unpkg fails', async () => {}); + test('No script source if downloading from both CDNs fail', async () => {}); }); }); }); From a14e77b4008e207aa9073570803948ef9e55bf3a Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Mon, 20 Apr 2020 17:05:05 -0700 Subject: [PATCH 02/12] Tests passing --- ...cdnWidgetScriptSourceProvider.unit.test.ts | 56 ++++++++++++++----- 1 file changed, 41 insertions(+), 15 deletions(-) diff --git a/src/test/datascience/ipywidgets/cdnWidgetScriptSourceProvider.unit.test.ts b/src/test/datascience/ipywidgets/cdnWidgetScriptSourceProvider.unit.test.ts index df6a3d7377d3..4fcd69935ad0 100644 --- a/src/test/datascience/ipywidgets/cdnWidgetScriptSourceProvider.unit.test.ts +++ b/src/test/datascience/ipywidgets/cdnWidgetScriptSourceProvider.unit.test.ts @@ -2,12 +2,14 @@ // 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 os from 'os'; 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'; @@ -15,6 +17,7 @@ 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 { IPyWidgetScriptSource } from '../../../client/datascience/ipywidgets/ipyWidgetScriptSource'; import { IWidgetScriptSourceProvider, WidgetScriptSource } from '../../../client/datascience/ipywidgets/types'; @@ -23,6 +26,7 @@ import { IJupyterConnection, ILocalResourceUriConverter, INotebook } from '../.. // 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/'; @@ -51,6 +55,10 @@ suite('DataScience - ipywidget - CDN', () => { 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); CDNWidgetScriptSourceProvider.validUrls = new Map(); scriptSourceProvider = new CDNWidgetScriptSourceProvider( instance(configService), @@ -60,6 +68,10 @@ suite('DataScience - ipywidget - CDN', () => { ); }); + shutdown(() => { + clearDiskCache(); + }); + function createStreamFromString(str: string) { const readable = new Readable(); readable._read = noop; @@ -70,7 +82,15 @@ suite('DataScience - ipywidget - CDN', () => { function createTemporaryFile(ext: string) { tempFileCount += 1; - const filePath = path.join(os.tmpdir(), `tempfile_for_test${tempFileCount}.${ext}`); + + // 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: () => { @@ -79,6 +99,16 @@ suite('DataScience - ipywidget - CDN', () => { }; } + 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() { + fs.removeSync(path.join(EXTENSION_ROOT_DIR, 'tmp', 'scripts')); + fs.removeSync(path.join(EXTENSION_ROOT_DIR, 'tmp', 'tempfile_loc')); + } + [true, false].forEach((localLaunch) => { suite(localLaunch ? 'Local Jupyter Server' : 'Remote Jupyter Server', () => { setup(() => { @@ -113,6 +143,7 @@ suite('DataScience - ipywidget - CDN', () => { let expectedSource = ''; let baseUrl = ''; let getUrl = ''; + let scriptUri = ''; setup(() => { baseUrl = cdn === 'unpkg.com' ? unpgkUrl : jsdelivrUrl; getUrl = @@ -120,33 +151,28 @@ suite('DataScience - ipywidget - CDN', () => { ? `${moduleName}@${moduleVersion}/dist/index` : `${moduleName}@${moduleVersion}/dist/index.js`; expectedSource = `${baseUrl}${getUrl}`; + scriptUri = generateScriptName(moduleName, moduleVersion); CDNWidgetScriptSourceProvider.validUrls = new Map(); }); - test('Get widget source from CDN', async () => { - updateCDNSettings(cdn); - - const value = await scriptSourceProvider.getWidgetScriptSource(moduleName, moduleVersion); - - assert.deepEqual(value, { - moduleName: 'HelloWorld', - scriptUri: expectedSource, - source: 'cdn' - }); + teardown(() => { + clearDiskCache(); }); test('Ensure widget script is downloaded once and cached', async () => { + updateCDNSettings(cdn); let downloadCount = 0; nock(baseUrl) - .get(getUrl) + .get(`/${getUrl}`) .reply(200, () => { downloadCount += 1; return createStreamFromString('foo'); }); + when(httpClient.exists(anything())).thenResolve(true); const value = await scriptSourceProvider.getWidgetScriptSource(moduleName, moduleVersion); assert.deepEqual(value, { moduleName: 'HelloWorld', - scriptUri: expectedSource, + scriptUri, source: 'cdn' }); @@ -154,7 +180,7 @@ suite('DataScience - ipywidget - CDN', () => { assert.deepEqual(value2, { moduleName: 'HelloWorld', - scriptUri: expectedSource, + scriptUri, source: 'cdn' }); From 50233e19d02a455b3748f8e2ad80602dc8432e22 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Mon, 20 Apr 2020 17:21:58 -0700 Subject: [PATCH 03/12] Add more tests --- .../cdnWidgetScriptSourceProvider.ts | 2 +- ...cdnWidgetScriptSourceProvider.unit.test.ts | 69 +++++++++++++++++-- 2 files changed, 65 insertions(+), 6 deletions(-) diff --git a/src/client/datascience/ipywidgets/cdnWidgetScriptSourceProvider.ts b/src/client/datascience/ipywidgets/cdnWidgetScriptSourceProvider.ts index 0ca293830ae5..0397d65872e2 100644 --- a/src/client/datascience/ipywidgets/cdnWidgetScriptSourceProvider.ts +++ b/src/client/datascience/ipywidgets/cdnWidgetScriptSourceProvider.ts @@ -83,7 +83,7 @@ export class CDNWidgetScriptSourceProvider implements IWidgetScriptSourceProvide private readonly fileSystem: IFileSystem ) {} public dispose() { - // Noop. + this.cache.clear(); } public async getWidgetScriptSource(moduleName: string, moduleVersion: string): Promise { // First see if we already have it downloaded. diff --git a/src/test/datascience/ipywidgets/cdnWidgetScriptSourceProvider.unit.test.ts b/src/test/datascience/ipywidgets/cdnWidgetScriptSourceProvider.unit.test.ts index 4fcd69935ad0..42abc9daf44d 100644 --- a/src/test/datascience/ipywidgets/cdnWidgetScriptSourceProvider.unit.test.ts +++ b/src/test/datascience/ipywidgets/cdnWidgetScriptSourceProvider.unit.test.ts @@ -156,6 +156,8 @@ suite('DataScience - ipywidget - CDN', () => { }); teardown(() => { clearDiskCache(); + scriptSourceProvider.dispose(); + nock.cleanAll(); }); test('Ensure widget script is downloaded once and cached', async () => { updateCDNSettings(cdn); @@ -186,11 +188,68 @@ suite('DataScience - ipywidget - CDN', () => { assert.equal(downloadCount, 1, 'Downloaded more than once'); }); - test('No script source if package does not exist on CDN', async () => {}); - test('No script source if package does not exist on both CDNs', async () => {}); - test('Get Script from unpk if jsdelivr fails', async () => {}); - test('Get Script from jsdelivr if unpkg fails', async () => {}); - test('No script source if downloading from both CDNs fail', async () => {}); + test('No script source if package does not exist on CDN', async () => { + updateCDNSettings(cdn); + nock(baseUrl) + .get(`/${getUrl}`) + .reply(200, () => { + return createStreamFromString('foo'); + }); + when(httpClient.exists(anything())).thenResolve(false); + + const value = await scriptSourceProvider.getWidgetScriptSource(moduleName, moduleVersion); + + assert.deepEqual(value, { + moduleName: 'HelloWorld' + }); + }); + 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, + source: 'cdn' + }); + }); + + test('Script source if package already on disk', async () => { + updateCDNSettings(cdn); + // Make nobody available + when(httpClient.exists(anything())).thenResolve(false); + + // 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, + source: 'cdn' + }); + }); }); }); }); From 06a19005aca1963d9531e7862f52f783c6684a02 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Mon, 20 Apr 2020 17:53:15 -0700 Subject: [PATCH 04/12] Add retry test --- news/2 Fixes/11274.md | 1 + ...cdnWidgetScriptSourceProvider.unit.test.ts | 30 ++++++++++++++++--- 2 files changed, 27 insertions(+), 4 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/test/datascience/ipywidgets/cdnWidgetScriptSourceProvider.unit.test.ts b/src/test/datascience/ipywidgets/cdnWidgetScriptSourceProvider.unit.test.ts index 42abc9daf44d..4f880403fdce 100644 --- a/src/test/datascience/ipywidgets/cdnWidgetScriptSourceProvider.unit.test.ts +++ b/src/test/datascience/ipywidgets/cdnWidgetScriptSourceProvider.unit.test.ts @@ -5,7 +5,6 @@ import * as fs from 'fs-extra'; import { sha256 } from 'hash.js'; import { shutdown } from 'log4js'; import * as nock from 'nock'; -import * as os from 'os'; import * as path from 'path'; import { Readable } from 'stream'; import { anything, instance, mock, verify, when } from 'ts-mockito'; @@ -20,7 +19,7 @@ import { noop } from '../../../client/common/utils/misc'; import { EXTENSION_ROOT_DIR } from '../../../client/constants'; import { CDNWidgetScriptSourceProvider } from '../../../client/datascience/ipywidgets/cdnWidgetScriptSourceProvider'; import { IPyWidgetScriptSource } from '../../../client/datascience/ipywidgets/ipyWidgetScriptSource'; -import { IWidgetScriptSourceProvider, WidgetScriptSource } from '../../../client/datascience/ipywidgets/types'; +import { IWidgetScriptSourceProvider } from '../../../client/datascience/ipywidgets/types'; import { JupyterNotebookBase } from '../../../client/datascience/jupyter/jupyterNotebook'; import { IJupyterConnection, ILocalResourceUriConverter, INotebook } from '../../../client/datascience/types'; @@ -231,10 +230,33 @@ suite('DataScience - ipywidget - CDN', () => { }); }); - test('Script source if package already on disk', async () => { + test('Retry if busy', async () => { + let retryCount = 0; + updateCDNSettings(cdn); + when(httpClient.exists(anything())).thenResolve(true); + nock(baseUrl) + .get(`/${getUrl}`) + .reply(() => { + retryCount += 1; + if (retryCount > 2) { + return createStreamFromString('foo'); + } + return null; + }); + + // Then see if we can get it still. + const value = await scriptSourceProvider.getWidgetScriptSource(moduleName, moduleVersion); + + assert.deepEqual(value, { + moduleName: 'HelloWorld', + scriptUri, + source: 'cdn' + }); + }); + test('Script source already on disk', async () => { updateCDNSettings(cdn); // Make nobody available - when(httpClient.exists(anything())).thenResolve(false); + when(httpClient.exists(anything())).thenResolve(true); // Write to where the file should eventually end up const filePath = Uri.parse(scriptUri).fsPath; From 9cba0f727e1de5225773992efeeb30f2161fb753 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Mon, 20 Apr 2020 18:02:04 -0700 Subject: [PATCH 05/12] Fix build errors. Not sure why not happening locally. --- .../ipywidgets/cdnWidgetScriptSourceProvider.unit.test.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/test/datascience/ipywidgets/cdnWidgetScriptSourceProvider.unit.test.ts b/src/test/datascience/ipywidgets/cdnWidgetScriptSourceProvider.unit.test.ts index 4f880403fdce..1a8d4f365d00 100644 --- a/src/test/datascience/ipywidgets/cdnWidgetScriptSourceProvider.unit.test.ts +++ b/src/test/datascience/ipywidgets/cdnWidgetScriptSourceProvider.unit.test.ts @@ -139,7 +139,6 @@ suite('DataScience - ipywidget - CDN', () => { suite(cdn, () => { const moduleName = 'HelloWorld'; const moduleVersion = '1'; - let expectedSource = ''; let baseUrl = ''; let getUrl = ''; let scriptUri = ''; @@ -149,7 +148,6 @@ suite('DataScience - ipywidget - CDN', () => { cdn === 'unpkg.com' ? `${moduleName}@${moduleVersion}/dist/index` : `${moduleName}@${moduleVersion}/dist/index.js`; - expectedSource = `${baseUrl}${getUrl}`; scriptUri = generateScriptName(moduleName, moduleVersion); CDNWidgetScriptSourceProvider.validUrls = new Map(); }); From d69f320ea4fdcaa0e0094c67f18a61e54f844e4e Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Tue, 21 Apr 2020 09:19:47 -0700 Subject: [PATCH 06/12] Fix unit tests --- src/test/datascience/jupyter/jupyterSession.unit.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/test/datascience/jupyter/jupyterSession.unit.test.ts b/src/test/datascience/jupyter/jupyterSession.unit.test.ts index 9f974352027d..b53e03dbd94b 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 8423847c7a3f344ad0fa54f8b7e07d81c5bba3ff Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Tue, 21 Apr 2020 11:00:44 -0700 Subject: [PATCH 07/12] Fix azure ml on windows Rework error checking to use status --- .../cdnWidgetScriptSourceProvider.ts | 81 ++++++++++--------- .../ipywidgets/ipyWidgetScriptSource.ts | 10 +-- .../localWidgetScriptSourceProvider.ts | 4 +- ...cdnWidgetScriptSourceProvider.unit.test.ts | 20 +++-- 4 files changed, 60 insertions(+), 55 deletions(-) diff --git a/src/client/datascience/ipywidgets/cdnWidgetScriptSourceProvider.ts b/src/client/datascience/ipywidgets/cdnWidgetScriptSourceProvider.ts index 0397d65872e2..cf3172a39f7a 100644 --- a/src/client/datascience/ipywidgets/cdnWidgetScriptSourceProvider.ts +++ b/src/client/datascience/ipywidgets/cdnWidgetScriptSourceProvider.ts @@ -90,6 +90,7 @@ export class CDNWidgetScriptSourceProvider implements IWidgetScriptSourceProvide 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) { @@ -102,15 +103,15 @@ export class CDNWidgetScriptSourceProvider implements IWidgetScriptSourceProvide // If still not found, download it. if (!cached) { - // 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. - const file = await this.downloadFastestCDN(moduleName, moduleVersion); try { - if (file) { + // 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(file.filePath, diskPath); + 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 = ( @@ -124,8 +125,8 @@ export class CDNWidgetScriptSourceProvider implements IWidgetScriptSourceProvide traceError('Error downloading from CDN: ', exc); cached = { moduleName }; } finally { - if (file) { - file.dispose(); + if (tempFile) { + tempFile.dispose(); } } this.cache.set(key, cached); @@ -183,8 +184,11 @@ export class CDNWidgetScriptSourceProvider implements IWidgetScriptSourceProvide if (cdnBaseUrl) { // May have already been validated. const url = moduleNameToCDNUrl(cdnBaseUrl, moduleName, moduleVersion); - if (CDNWidgetScriptSourceProvider.validUrls.has(url)) { + const checkResult = CDNWidgetScriptSourceProvider.validUrls.get(url); + if (checkResult) { return url; + } else if (checkResult !== undefined) { + return undefined; } // Try pinging first. @@ -201,12 +205,32 @@ export class CDNWidgetScriptSourceProvider implements IWidgetScriptSourceProvide return sanitize(sha256().update(`${moduleName}${moduleVersion}`).digest('hex')); } - private async downloadFile(downloadUrl: string | undefined): Promise { - // Download URL should not be undefined (we validated in a filter above) - if (!downloadUrl) { - throw new Error('Cannot download from an undefined CDN'); - } + 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'); @@ -217,36 +241,13 @@ export class CDNWidgetScriptSourceProvider implements IWidgetScriptSourceProvide let req: request.Request; try { req = await this.httpClient.downloadFile(downloadUrl); + success = await this.handleResponse(req, tempFile.filePath); + retryCount -= 1; } catch (exc) { traceInfo(`Error downloading from ${downloadUrl}: `, exc); retryCount -= 1; - - // CDN may be busy, give it a break. - await sleep(500); continue; } - - try { - if (req) { - // Write to our temp file. - const tempWriteStream = this.fileSystem.createWriteStream(tempFile.filePath); - const deferred = createDeferred(); - req.on('error', (e) => { - traceError(`Error downloading from CDN: `, e); - deferred.reject(e); - }) - .pipe(tempWriteStream) - .on('close', () => { - deferred.resolve(); - }); - await deferred.promise; - success = true; - } - } catch (exc) { - // Don't do anything if we fail to write it. - traceError(`Error writing CDN download to disk: `, exc); - break; - } } // Once we make it out, return result diff --git a/src/client/datascience/ipywidgets/ipyWidgetScriptSource.ts b/src/client/datascience/ipywidgets/ipyWidgetScriptSource.ts index 4ce415714c5a..f0ece0610cd2 100644 --- a/src/client/datascience/ipywidgets/ipyWidgetScriptSource.ts +++ b/src/client/datascience/ipywidgets/ipyWidgetScriptSource.ts @@ -66,7 +66,7 @@ export class IPyWidgetScriptSource implements IInteractiveWindowListener, ILocal private pendingModuleRequests = new Map(); private readonly uriConversionPromises = new Map>(); private readonly targetWidgetScriptsFolder: string; - private readonly rootScriptsFolder: string; + private readonly _rootScriptFolder: string; private readonly createTargetWidgetScriptsFolder: Promise; constructor( @inject(IDisposableRegistry) disposables: IDisposableRegistry, @@ -80,8 +80,8 @@ export class IPyWidgetScriptSource implements IInteractiveWindowListener, ILocal @inject(IPersistentStateFactory) private readonly stateFactory: IPersistentStateFactory, @inject(IExtensionContext) extensionContext: IExtensionContext ) { - this.rootScriptsFolder = path.join(extensionContext.extensionPath, 'tmp', 'scripts'); - this.targetWidgetScriptsFolder = path.join(this.rootScriptsFolder, '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) => { @@ -111,7 +111,7 @@ export class IPyWidgetScriptSource implements IInteractiveWindowListener, ILocal */ public async asWebviewUri(localResource: Uri): Promise { // Make a copy of the local file if not already in the correct location - if (!localResource.fsPath.startsWith(this.rootScriptsFolder)) { + if (!localResource.fsPath.startsWith(this._rootScriptFolder)) { if (this.notebookIdentity && !this.resourcesMappedToExtensionFolder.has(localResource.fsPath)) { const deferred = createDeferred(); this.resourcesMappedToExtensionFolder.set(localResource.fsPath, deferred.promise); @@ -151,7 +151,7 @@ export class IPyWidgetScriptSource implements IInteractiveWindowListener, ILocal } public get rootScriptFolder(): Uri { - return Uri.file(this.rootScriptsFolder); + return Uri.file(this._rootScriptFolder); } public dispose() { 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/test/datascience/ipywidgets/cdnWidgetScriptSourceProvider.unit.test.ts b/src/test/datascience/ipywidgets/cdnWidgetScriptSourceProvider.unit.test.ts index 1a8d4f365d00..586491d4a6c4 100644 --- a/src/test/datascience/ipywidgets/cdnWidgetScriptSourceProvider.unit.test.ts +++ b/src/test/datascience/ipywidgets/cdnWidgetScriptSourceProvider.unit.test.ts @@ -104,8 +104,12 @@ suite('DataScience - ipywidget - CDN', () => { } function clearDiskCache() { - fs.removeSync(path.join(EXTENSION_ROOT_DIR, 'tmp', 'scripts')); - fs.removeSync(path.join(EXTENSION_ROOT_DIR, 'tmp', 'tempfile_loc')); + 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) => { @@ -232,14 +236,13 @@ suite('DataScience - ipywidget - CDN', () => { let retryCount = 0; updateCDNSettings(cdn); when(httpClient.exists(anything())).thenResolve(true); + nock(baseUrl).get(`/${getUrl}`).twice().replyWithError('Not found'); nock(baseUrl) .get(`/${getUrl}`) - .reply(() => { - retryCount += 1; - if (retryCount > 2) { - return createStreamFromString('foo'); - } - return null; + .thrice() + .reply(200, () => { + retryCount = 3; + return createStreamFromString('foo'); }); // Then see if we can get it still. @@ -250,6 +253,7 @@ suite('DataScience - ipywidget - CDN', () => { scriptUri, source: 'cdn' }); + assert.equal(retryCount, 3, 'Did not actually retry'); }); test('Script source already on disk', async () => { updateCDNSettings(cdn); From 13773b66cddf191b0f06bdf08dc8b8932e5eab47 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Tue, 21 Apr 2020 11:03:58 -0700 Subject: [PATCH 08/12] Fix sonar error --- src/client/datascience/jupyter/jupyterSession.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client/datascience/jupyter/jupyterSession.ts b/src/client/datascience/jupyter/jupyterSession.ts index 3dceef14b58b..7e69a365ebb9 100644 --- a/src/client/datascience/jupyter/jupyterSession.ts +++ b/src/client/datascience/jupyter/jupyterSession.ts @@ -132,7 +132,7 @@ export class JupyterSession extends BaseJupyterSession { // This is just like doing a restart, kill the old session (and the old restart session), and start new ones if (this.session) { this.shutdownSession(this.session, this.statusHandler).ignoreErrors(); - this.restartSessionPromise?.then((r) => this.shutdownSession(r, undefined)).ignoreErrors(); + this.restartSessionPromise?.then((r) => this.shutdownSession(r, undefined)).ignoreErrors(); // NOSONAR } // Update our kernel spec From d45f86317f599213af1767a9cbc35e82ebd69c26 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Tue, 21 Apr 2020 11:06:09 -0700 Subject: [PATCH 09/12] Add some more descriptive comments --- .../ipywidgets/cdnWidgetScriptSourceProvider.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/client/datascience/ipywidgets/cdnWidgetScriptSourceProvider.ts b/src/client/datascience/ipywidgets/cdnWidgetScriptSourceProvider.ts index cf3172a39f7a..606047079f88 100644 --- a/src/client/datascience/ipywidgets/cdnWidgetScriptSourceProvider.ts +++ b/src/client/datascience/ipywidgets/cdnWidgetScriptSourceProvider.ts @@ -141,7 +141,9 @@ export class CDNWidgetScriptSourceProvider implements IWidgetScriptSourceProvide // For each CDN, try to download it. this.cdnProviders.map((cdn) => this.downloadFromCDN(moduleName, moduleVersion, cdn).then((t) => { - // First one to get here wins. + // 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); } @@ -149,7 +151,8 @@ export class CDNWidgetScriptSourceProvider implements IWidgetScriptSourceProvide ) ) .then((_a) => { - // If still not resolved, then return empty + // 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); } From 12903f9d1c63d171e43dfc708c64fd186203b748 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Tue, 21 Apr 2020 11:07:10 -0700 Subject: [PATCH 10/12] More comments --- .../datascience/ipywidgets/cdnWidgetScriptSourceProvider.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/client/datascience/ipywidgets/cdnWidgetScriptSourceProvider.ts b/src/client/datascience/ipywidgets/cdnWidgetScriptSourceProvider.ts index 606047079f88..380c33743312 100644 --- a/src/client/datascience/ipywidgets/cdnWidgetScriptSourceProvider.ts +++ b/src/client/datascience/ipywidgets/cdnWidgetScriptSourceProvider.ts @@ -158,6 +158,9 @@ export class CDNWidgetScriptSourceProvider implements IWidgetScriptSourceProvide } }) .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; } From 9c5c4a85081ab79386259863c11ea12e654b4069 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Tue, 21 Apr 2020 12:39:19 -0700 Subject: [PATCH 11/12] Refactor some code --- .../datascience/ipywidgets/cdnWidgetScriptSourceProvider.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/client/datascience/ipywidgets/cdnWidgetScriptSourceProvider.ts b/src/client/datascience/ipywidgets/cdnWidgetScriptSourceProvider.ts index 380c33743312..f5256d255ea3 100644 --- a/src/client/datascience/ipywidgets/cdnWidgetScriptSourceProvider.ts +++ b/src/client/datascience/ipywidgets/cdnWidgetScriptSourceProvider.ts @@ -248,11 +248,10 @@ export class CDNWidgetScriptSourceProvider implements IWidgetScriptSourceProvide try { req = await this.httpClient.downloadFile(downloadUrl); success = await this.handleResponse(req, tempFile.filePath); - retryCount -= 1; } catch (exc) { traceInfo(`Error downloading from ${downloadUrl}: `, exc); + } finally { retryCount -= 1; - continue; } } From ccb5f62f64879e756abb6457b11b7d4ecb704dd5 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Tue, 21 Apr 2020 15:28:57 -0700 Subject: [PATCH 12/12] Make sure to not use exists as we're downloading anyway --- .../cdnWidgetScriptSourceProvider.ts | 20 +------------------ ...cdnWidgetScriptSourceProvider.unit.test.ts | 10 +--------- ...ipyWidgetScriptSourceProvider.unit.test.ts | 1 - 3 files changed, 2 insertions(+), 29 deletions(-) diff --git a/src/client/datascience/ipywidgets/cdnWidgetScriptSourceProvider.ts b/src/client/datascience/ipywidgets/cdnWidgetScriptSourceProvider.ts index f5256d255ea3..a54d22f6a451 100644 --- a/src/client/datascience/ipywidgets/cdnWidgetScriptSourceProvider.ts +++ b/src/client/datascience/ipywidgets/cdnWidgetScriptSourceProvider.ts @@ -12,9 +12,6 @@ import { traceError, traceInfo } from '../../common/logger'; import { IFileSystem, TemporaryFile } from '../../common/platform/types'; import { IConfigurationService, IHttpClient, WidgetCDNs } from '../../common/types'; import { createDeferred, sleep } from '../../common/utils/async'; -import { StopWatch } from '../../common/utils/stopWatch'; -import { sendTelemetryEvent } from '../../telemetry'; -import { Telemetry } from '../constants'; import { ILocalResourceUriConverter } from '../types'; import { IWidgetScriptSourceProvider, WidgetScriptSource } from './types'; @@ -74,7 +71,6 @@ 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, @@ -188,21 +184,7 @@ export class CDNWidgetScriptSourceProvider implements IWidgetScriptSourceProvide ): Promise { const cdnBaseUrl = getCDNPrefix(cdn); if (cdnBaseUrl) { - // May have already been validated. - const url = moduleNameToCDNUrl(cdnBaseUrl, moduleName, moduleVersion); - const checkResult = CDNWidgetScriptSourceProvider.validUrls.get(url); - if (checkResult) { - return url; - } else if (checkResult !== undefined) { - return undefined; - } - - // Try pinging first. - 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 ? url : undefined; + return moduleNameToCDNUrl(cdnBaseUrl, moduleName, moduleVersion); } return undefined; } diff --git a/src/test/datascience/ipywidgets/cdnWidgetScriptSourceProvider.unit.test.ts b/src/test/datascience/ipywidgets/cdnWidgetScriptSourceProvider.unit.test.ts index 586491d4a6c4..92e5083fc7ec 100644 --- a/src/test/datascience/ipywidgets/cdnWidgetScriptSourceProvider.unit.test.ts +++ b/src/test/datascience/ipywidgets/cdnWidgetScriptSourceProvider.unit.test.ts @@ -58,7 +58,6 @@ suite('DataScience - ipywidget - CDN', () => { Uri.file(path.join(EXTENSION_ROOT_DIR, 'tmp', 'scripts')) ); when(webviewUriConverter.asWebviewUri(anything())).thenCall((u) => u); - CDNWidgetScriptSourceProvider.validUrls = new Map(); scriptSourceProvider = new CDNWidgetScriptSourceProvider( instance(configService), instance(httpClient), @@ -153,7 +152,6 @@ suite('DataScience - ipywidget - CDN', () => { ? `${moduleName}@${moduleVersion}/dist/index` : `${moduleName}@${moduleVersion}/dist/index.js`; scriptUri = generateScriptName(moduleName, moduleVersion); - CDNWidgetScriptSourceProvider.validUrls = new Map(); }); teardown(() => { clearDiskCache(); @@ -169,7 +167,6 @@ suite('DataScience - ipywidget - CDN', () => { downloadCount += 1; return createStreamFromString('foo'); }); - when(httpClient.exists(anything())).thenResolve(true); const value = await scriptSourceProvider.getWidgetScriptSource(moduleName, moduleVersion); @@ -191,12 +188,7 @@ suite('DataScience - ipywidget - CDN', () => { }); test('No script source if package does not exist on CDN', async () => { updateCDNSettings(cdn); - nock(baseUrl) - .get(`/${getUrl}`) - .reply(200, () => { - return createStreamFromString('foo'); - }); - when(httpClient.exists(anything())).thenResolve(false); + nock(baseUrl).get(`/${getUrl}`).replyWithError('404'); const value = await scriptSourceProvider.getWidgetScriptSource(moduleName, moduleVersion); diff --git a/src/test/datascience/ipywidgets/ipyWidgetScriptSourceProvider.unit.test.ts b/src/test/datascience/ipywidgets/ipyWidgetScriptSourceProvider.unit.test.ts index 208efe72e418..7d90dc1c3c6f 100644 --- a/src/test/datascience/ipywidgets/ipyWidgetScriptSourceProvider.unit.test.ts +++ b/src/test/datascience/ipywidgets/ipyWidgetScriptSourceProvider.unit.test.ts @@ -54,7 +54,6 @@ suite('Data 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),