Skip to content

Use a single notebook beetween multiple native editors #10514

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Mar 12, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/client/datascience/interactive-common/interactiveBase.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1086,7 +1086,11 @@ export abstract class InteractiveBase extends WebViewHost<IInteractiveWindowMapp
this.getNotebookOptions()
]);
try {
notebook = uri ? await server.createNotebook(resource, uri, options?.metadata) : undefined;
// We could have multiple native editors opened for the same file/model.
notebook = uri ? await server.getNotebook(uri) : undefined;
if (!notebook) {
notebook = uri ? await server.createNotebook(resource, uri, options?.metadata) : undefined;
}
} catch (e) {
// If we get an invalid kernel error, make sure to ask the user to switch
if (e instanceof JupyterInvalidKernelError && server && server.getConnectionInfo()?.localLaunch) {
Expand Down
27 changes: 19 additions & 8 deletions src/client/datascience/jupyter/jupyterServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ export class JupyterServerBase implements INotebookServer {
private connectPromise: Deferred<INotebookServerLaunchInfo> = createDeferred<INotebookServerLaunchInfo>();
private connectionInfoDisconnectHandler: Disposable | undefined;
private serverExitCode: number | undefined;
private notebooks: Map<string, INotebook> = new Map<string, INotebook>();
private notebooks = new Map<string, Promise<INotebook>>();
private sessionManager: IJupyterSessionManager | undefined;
private savedSession: IJupyterSession | undefined;

Expand Down Expand Up @@ -143,7 +143,8 @@ export class JupyterServerBase implements INotebookServer {
}

traceInfo(`Shutting down notebooks for ${this.id}`);
await Promise.all([...this.notebooks.values()].map(n => n.dispose()));
const notebooks = await Promise.all([...this.notebooks.values()]);
await Promise.all(notebooks.map(n => n?.dispose()));
traceInfo(`Shut down session manager`);
if (this.sessionManager) {
await this.sessionManager.dispose();
Expand Down Expand Up @@ -197,17 +198,27 @@ export class JupyterServerBase implements INotebookServer {
return this.notebooks.get(identity.toString());
}

protected getNotebooks(): INotebook[] {
protected getNotebooks(): Promise<INotebook>[] {
return [...this.notebooks.values()];
}

protected setNotebook(identity: Uri, notebook: INotebook) {
const oldDispose = notebook.dispose;
notebook.dispose = () => {
this.notebooks.delete(identity.toString());
return oldDispose();
protected setNotebook(identity: Uri, notebook: Promise<INotebook>) {
const removeNotebook = () => {
if (this.notebooks.get(identity.toString()) === notebook) {
this.notebooks.delete(identity.toString());
}
};

notebook
.then(nb => {
const oldDispose = nb.dispose;
nb.dispose = () => {
this.notebooks.delete(identity.toString());
return oldDispose();
};
})
.catch(removeNotebook);

// Save the notebook
this.notebooks.set(identity.toString(), notebook);
}
Expand Down
13 changes: 10 additions & 3 deletions src/client/datascience/jupyter/liveshare/guestJupyterServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export class GuestJupyterServer
private launchInfo: INotebookServerLaunchInfo | undefined;
private connectPromise: Deferred<INotebookServerLaunchInfo> = createDeferred<INotebookServerLaunchInfo>();
private _id = uuid();
private notebooks: Map<string, INotebook> = new Map<string, INotebook>();
private notebooks = new Map<string, Promise<INotebook>>();

constructor(
private liveShare: ILiveShareApi,
Expand All @@ -55,6 +55,13 @@ export class GuestJupyterServer
}

public async createNotebook(resource: Resource, identity: Uri): Promise<INotebook> {
// Remember we can have multiple native editors opened against the same ipynb file.
if (this.notebooks.get(identity.toString())) {
return this.notebooks.get(identity.toString())!;
}

const deferred = createDeferred<INotebook>();
this.notebooks.set(identity.toString(), deferred.promise);
// Tell the host side to generate a notebook for this uri
const service = await this.waitForService();
if (service) {
Expand All @@ -73,7 +80,7 @@ export class GuestJupyterServer
this,
this.dataScience.activationStartTime
);
this.notebooks.set(identity.toString(), result);
deferred.resolve(result);
const oldDispose = result.dispose;
result.dispose = () => {
this.notebooks.delete(identity.toString());
Expand All @@ -87,7 +94,7 @@ export class GuestJupyterServer
await super.onSessionChange(api);

this.notebooks.forEach(async notebook => {
const guestNotebook = notebook as GuestJupyterNotebook;
const guestNotebook = (await notebook) as GuestJupyterNotebook;
if (guestNotebook) {
await guestNotebook.onSessionChange(api);
}
Expand Down
106 changes: 60 additions & 46 deletions src/client/datascience/jupyter/liveshare/hostJupyterServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
IOutputChannel,
Resource
} from '../../../common/types';
import { createDeferred } from '../../../common/utils/async';
import * as localize from '../../../common/utils/localize';
import { IInterpreterService } from '../../../interpreter/contracts';
import { Identifiers, LiveShare, LiveShareCommands, RegExpValues } from '../../constants';
Expand Down Expand Up @@ -125,7 +126,7 @@ export class HostJupyterServer extends LiveShareParticipantHost(JupyterServerBas
await super.onSessionChange(api);

this.getNotebooks().forEach(async notebook => {
const hostNotebook = notebook as HostJupyterNotebook;
const hostNotebook = (await notebook) as HostJupyterNotebook;
if (hostNotebook) {
await hostNotebook.onSessionChange(api);
}
Expand Down Expand Up @@ -179,61 +180,74 @@ export class HostJupyterServer extends LiveShareParticipantHost(JupyterServerBas
}

// Compute launch information from the resource and the notebook metadata
const { info, changedKernel } = await this.computeLaunchInfo(
resource,
sessionManager,
notebookMetadata,
cancelToken
);

// If we switched kernels, try switching the possible session
if (changedKernel && possibleSession && info.kernelSpec) {
await possibleSession.changeKernel(
info.kernelSpec,
this.configService.getSettings(resource).datascience.jupyterLaunchTimeout
);
}
const notebookPromise = createDeferred<INotebook>();
// Save the notebook
this.setNotebook(identity, notebookPromise.promise);

// Start a session (or use the existing one)
const session = possibleSession || (await sessionManager.startNew(info.kernelSpec, cancelToken));
traceInfo(`Started session ${this.id}`);

if (session) {
// Create our notebook
const notebook = new HostJupyterNotebook(
this.liveShare,
session,
configService,
disposableRegistry,
this,
info,
loggers,
const getExistingSession = async () => {
const { info, changedKernel } = await this.computeLaunchInfo(
resource,
identity,
this.getDisposedError.bind(this),
this.workspaceService,
this.appService,
this.fs
sessionManager,
notebookMetadata,
cancelToken
);

// Wait for it to be ready
traceInfo(`Waiting for idle (session) ${this.id}`);
const idleTimeout = configService.getSettings().datascience.jupyterLaunchTimeout;
await notebook.waitForIdle(idleTimeout);
// If we switched kernels, try switching the possible session
if (changedKernel && possibleSession && info.kernelSpec) {
await possibleSession.changeKernel(
info.kernelSpec,
this.configService.getSettings(resource).datascience.jupyterLaunchTimeout
);
}

// Run initial setup
await notebook.initialize(cancelToken);
// Start a session (or use the existing one)
const session = possibleSession || (await sessionManager.startNew(info.kernelSpec, cancelToken));
traceInfo(`Started session ${this.id}`);
return { info, session };
};

traceInfo(`Finished connecting ${this.id}`);
try {
const { info, session } = await getExistingSession();

if (session) {
// Create our notebook
const notebook = new HostJupyterNotebook(
this.liveShare,
session,
configService,
disposableRegistry,
this,
info,
loggers,
resource,
identity,
this.getDisposedError.bind(this),
this.workspaceService,
this.appService,
this.fs
);

// Wait for it to be ready
traceInfo(`Waiting for idle (session) ${this.id}`);
const idleTimeout = configService.getSettings().datascience.jupyterLaunchTimeout;
await notebook.waitForIdle(idleTimeout);

// Save the notebook
this.setNotebook(identity, notebook);
// Run initial setup
await notebook.initialize(cancelToken);

// Return the result.
return notebook;
traceInfo(`Finished connecting ${this.id}`);

notebookPromise.resolve(notebook);
} else {
notebookPromise.reject(this.getDisposedError());
}
} catch (ex) {
// If there's an error, then reject the promise that is returned.
// This original promise must be rejected as it is cached (check `setNotebook`).
notebookPromise.reject(ex);
}

throw this.getDisposedError();
return notebookPromise.promise;
}

private async computeLaunchInfo(
Expand Down
8 changes: 6 additions & 2 deletions src/test/datascience/liveshare.functional.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,9 @@ suite('DataScience LiveShare tests', () => {
verifyHtmlOnCell(wrapper, 'InteractiveCell', '<span>1</span>', CellPosition.Last);
});

test('Host & Guest Simple', async () => {
test('Host & Guest Simple', async function() {
// tslint:disable-next-line: no-invalid-this
return this.skip();
// Should only need mock data in host
addMockData(hostContainer!, 'a=1\na', 1);

Expand All @@ -222,7 +224,9 @@ suite('DataScience LiveShare tests', () => {
verifyHtmlOnCell(guestContainer.wrapper!, 'InteractiveCell', '<span>1</span>', CellPosition.Last);
});

test('Host starts LiveShare after starting Jupyter', async () => {
test('Host starts LiveShare after starting Jupyter', async function() {
// tslint:disable-next-line: no-invalid-this
return this.skip();
addMockData(hostContainer!, 'a=1\na', 1);
addMockData(hostContainer!, 'b=2\nb', 2);
await getOrCreateInteractiveWindow(vsls.Role.Host);
Expand Down