Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Remove unnecessary nullables #9766

Merged
merged 4 commits into from
Apr 25, 2022
Merged

Remove unnecessary nullables #9766

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ba3b5b5
Misc
DonJayamanne Apr 25, 2022
0a73c6d
Remove nullables
DonJayamanne Apr 25, 2022
fdf0743
Misc
DonJayamanne Apr 25, 2022
1006723
Misc
DonJayamanne Apr 25, 2022
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
18 changes: 9 additions & 9 deletions src/kernels/jupyter/launcher/jupyterExecution.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,10 @@ import {
INotebookServerOptions,
INotebookServer,
JupyterServerUriHandle,
INotebookStarter
INotebookStarter,
INotebookServerFactory
} from '../types';
import { IJupyterSubCommandExecutionService } from '../types.node';
import { IServiceContainer } from '../../../platform/ioc/types';

const LocalHosts = ['localhost', '127.0.0.1', '::1'];

Expand All @@ -45,7 +45,7 @@ export class JupyterExecutionBase implements IJupyterExecution {
private readonly notebookStarter: INotebookStarter | undefined,
private readonly jupyterInterpreterService: IJupyterSubCommandExecutionService | undefined,
private readonly jupyterPickerRegistration: IJupyterUriProviderRegistration,
private readonly serviceContainer: IServiceContainer
private readonly notebookServerFactory: INotebookServerFactory
) {
this.disposableRegistry.push(this.interpreterService.onDidChangeInterpreter(() => this.onSettingsChanged()));
this.disposableRegistry.push(this);
Expand Down Expand Up @@ -101,7 +101,7 @@ export class JupyterExecutionBase implements IJupyterExecution {
public connectToNotebookServer(
options: INotebookServerOptions,
cancelToken: CancellationToken
): Promise<INotebookServer | undefined> {
): Promise<INotebookServer> {
// Return nothing if we cancel
// eslint-disable-next-line
return Cancellation.race(async () => {
Expand All @@ -122,12 +122,11 @@ export class JupyterExecutionBase implements IJupyterExecution {
if (!connection.localLaunch && LocalHosts.includes(connection.hostName.toLowerCase())) {
sendTelemetryEvent(Telemetry.ConnectRemoteJupyterViaLocalHost);
}
// Create a server tha t we will then attempt to connect to.
result = this.serviceContainer.get<INotebookServer>(INotebookServer);

// eslint-disable-next-line no-constant-condition
traceInfo(`Connecting to process server`);
await result.connect(connection, cancelToken);

// Create a server tha t we will then attempt to connect to.
result = await this.notebookServerFactory.createNotebookServer(connection);
traceInfo(`Connection complete server`);

sendTelemetryEvent(
Expand All @@ -152,7 +151,7 @@ export class JupyterExecutionBase implements IJupyterExecution {
} else if (connection) {
// If this is occurring during shutdown, don't worry about it.
Copy link
Contributor

@amunger amunger Apr 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

update comment? (Or just remove)

if (this.disposed) {
return undefined;
throw err;
}

// Something else went wrong
Expand Down Expand Up @@ -182,6 +181,7 @@ export class JupyterExecutionBase implements IJupyterExecution {
}
throw lastTryError;
}
throw new Error('Max number of attempts reached');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function was the root cause, we have an explicit return undefined and an implicit at the end of the while loop.
That causes return value to have undefined and bubbles all the way the to the top.
This is an unlikely scenario (cancellation is handled higher up, hence we don't need to swallow cancellations and disposing classes here)

}, cancelToken);
}

Expand Down
6 changes: 3 additions & 3 deletions src/kernels/jupyter/launcher/jupyterNotebookProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,15 @@ export class JupyterNotebookProvider implements IJupyterNotebookProvider {
@inject(IJupyterServerUriStorage) private readonly serverStorage: IJupyterServerUriStorage
) {}

public async connect(options: ConnectNotebookProviderOptions): Promise<IJupyterConnection | undefined> {
public async connect(options: ConnectNotebookProviderOptions): Promise<IJupyterConnection> {
const server = await this.serverProvider.getOrCreateServer({
ui: options.ui,
resource: options.resource,
token: options.token,
localJupyter: options.kind === 'localJupyter'
});
const connection = await server?.getConnectionInfo();
if (connection && options.kind === 'remoteJupyter') {
const connection = await server.connection;
if (options.kind === 'remoteJupyter') {
// Log this remote URI into our MRU list
void this.serverStorage.addToUriList(
connection.url || connection.displayName,
Expand Down
16 changes: 9 additions & 7 deletions src/kernels/jupyter/launcher/liveshare/hostJupyterExecution.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,13 @@ import {
} from '../../../../platform/common/types';
import { testOnlyMethod } from '../../../../platform/common/utils/decorators';
import { IInterpreterService } from '../../../../platform/interpreter/contracts';
import { IServiceContainer } from '../../../../platform/ioc/types';
import {
IJupyterExecution,
INotebookServerOptions,
INotebookServer,
INotebookStarter,
IJupyterUriProviderRegistration
IJupyterUriProviderRegistration,
INotebookServerFactory
} from '../../types';
import { IJupyterSubCommandExecutionService } from '../../types.node';

Expand All @@ -42,11 +42,11 @@ export class HostJupyterExecution extends JupyterExecutionBase implements IJupyt
@inject(IWorkspaceService) workspace: IWorkspaceService,
@inject(IConfigurationService) configService: IConfigurationService,
@inject(INotebookStarter) @optional() notebookStarter: INotebookStarter | undefined,
@inject(IServiceContainer) serviceContainer: IServiceContainer,
@inject(IJupyterSubCommandExecutionService)
@optional()
jupyterInterpreterService: IJupyterSubCommandExecutionService | undefined,
@inject(IJupyterUriProviderRegistration) jupyterPickerRegistration: IJupyterUriProviderRegistration
@inject(IJupyterUriProviderRegistration) jupyterPickerRegistration: IJupyterUriProviderRegistration,
@inject(INotebookServerFactory) notebookServerFactory: INotebookServerFactory
) {
super(
interpreterService,
Expand All @@ -56,7 +56,7 @@ export class HostJupyterExecution extends JupyterExecutionBase implements IJupyt
notebookStarter,
jupyterInterpreterService,
jupyterPickerRegistration,
serviceContainer
notebookServerFactory
);
this.serverCache = new ServerCache(workspace);
asyncRegistry.push(this);
Expand Down Expand Up @@ -85,19 +85,21 @@ export class HostJupyterExecution extends JupyterExecutionBase implements IJupyt
public async hostConnectToNotebookServer(
options: INotebookServerOptions,
cancelToken: CancellationToken
): Promise<INotebookServer | undefined> {
): Promise<INotebookServer> {
if (!this._disposed) {
return super.connectToNotebookServer(await this.serverCache.generateDefaultOptions(options), cancelToken);
}
throw new Error('Notebook server is disposed');
}

public override async connectToNotebookServer(
options: INotebookServerOptions,
cancelToken: CancellationToken
): Promise<INotebookServer | undefined> {
): Promise<INotebookServer> {
if (!this._disposed) {
return this.serverCache.getOrCreate(this.hostConnectToNotebookServer.bind(this), options, cancelToken);
}
throw new Error('Notebook server is disposed');
}
public override async getServer(options: INotebookServerOptions): Promise<INotebookServer | undefined> {
if (!this._disposed) {
Expand Down
72 changes: 19 additions & 53 deletions src/kernels/jupyter/launcher/liveshare/hostJupyterServer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import '../../../../platform/common/extensions';

import { CancellationToken } from 'vscode-jsonrpc';
import { injectable, inject, named } from 'inversify';
import { inject, named } from 'inversify';
import { IWorkspaceService } from '../../../../platform/common/application/types';
import { STANDARD_OUTPUT_CHANNEL } from '../../../../platform/common/constants';
import { traceInfo, traceError, traceInfoIfCI } from '../../../../platform/logging';
Expand All @@ -16,7 +16,7 @@ import {
IDisposable,
IDisplayOptions
} from '../../../../platform/common/types';
import { Deferred, createDeferred, sleep } from '../../../../platform/common/utils/async';
import { createDeferred, sleep } from '../../../../platform/common/utils/async';
import { DataScience } from '../../../../platform/common/utils/localize';
import { StopWatch } from '../../../../platform/common/utils/stopWatch';
import { SessionDisposedError } from '../../../../platform/errors/sessionDisposedError';
Expand All @@ -34,27 +34,34 @@ import { JupyterNotebook } from '../jupyterNotebook';
import { noop } from '../../../../platform/common/utils/misc';
import { Cancellation } from '../../../../platform/common/cancellation';
import { getDisplayPath } from '../../../../platform/common/platform/fs-paths';
import { INotebookServer, IJupyterSessionManagerFactory } from '../../types';
import { INotebookServer } from '../../types';
import { Uri } from 'vscode';
/* eslint-disable @typescript-eslint/no-explicit-any */

@injectable()
export class HostJupyterServer implements INotebookServer {
private connection: IJupyterConnection | undefined;
private connectPromise: Deferred<IJupyterConnection> = createDeferred<IJupyterConnection>();
private connectionInfoDisconnectHandler: IDisposable | undefined;
private serverExitCode: number | undefined;
private notebooks = new Set<Promise<INotebook>>();
private sessionManager: JupyterSessionManager | undefined;
private disposed = false;
constructor(
@inject(IAsyncDisposableRegistry) private readonly asyncRegistry: IAsyncDisposableRegistry,
@inject(IJupyterSessionManagerFactory) private readonly sessionManagerFactory: IJupyterSessionManagerFactory,
@inject(IWorkspaceService) private readonly workspaceService: IWorkspaceService,
@inject(IOutputChannel) @named(STANDARD_OUTPUT_CHANNEL) private readonly jupyterOutputChannel: IOutputChannel,
@inject(IDisposableRegistry) private readonly disposables: IDisposableRegistry
@inject(IDisposableRegistry) private readonly disposables: IDisposableRegistry,
public connection: IJupyterConnection,
private sessionManager: JupyterSessionManager
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The connection property is accessed using getConnection and we have some code that waits for this to be initialized.
Basically as soon as this class is contructed, the first method invoked is connect and its impossible for connection to be undefined, however due to the way the class was constructed it can be undefined in types & we have code all over the place that treats it as such.

Refactored this class to be created via a factory so that it can never be nullable, as a result the code paths are much simpler now.
No more nullables.

) {
this.asyncRegistry.push(this);

this.connectionInfoDisconnectHandler = this.connection.disconnected((c) => {
try {
this.serverExitCode = c;
traceError(DataScience.jupyterServerCrashed().format(c.toString()));
this.shutdown().ignoreErrors();
} catch {
noop();
}
});
}

public async dispose(): Promise<void> {
Expand Down Expand Up @@ -89,7 +96,7 @@ export class HostJupyterServer implements INotebookServer {
// Save the notebook
this.trackDisposable(notebookPromise.promise);
const getExistingSession = async () => {
const connection = await this.computeLaunchInfo();
const connection = this.connection;
this.throwIfDisposedOrCancelled(cancelToken);
// Figure out the working directory we need for our new notebook. This is only necessary for local.
const workingDirectory = isLocalConnection(kernelConnection)
Expand Down Expand Up @@ -131,41 +138,6 @@ export class HostJupyterServer implements INotebookServer {
return notebookPromise.promise;
}

private async computeLaunchInfo(): Promise<IJupyterConnection> {
// First we need our launch information so we can start a new session (that's what our notebook is really)
let launchInfo = await this.waitForConnect();
if (!launchInfo) {
throw this.getDisposedError();
}
return launchInfo;
}

public async connect(connection: IJupyterConnection, _cancelToken: CancellationToken): Promise<void> {
traceInfo(`Connecting server kernel ${connection.baseUrl}`);

// Save our launch info
this.connection = connection;

// Indicate connect started
this.connectPromise.resolve(connection);

this.connectionInfoDisconnectHandler = this.connection.disconnected((c) => {
try {
this.serverExitCode = c;
traceError(DataScience.jupyterServerCrashed().format(c.toString()));
this.shutdown().ignoreErrors();
} catch {
noop();
}
});

// Indicate we have a new session on the output channel
this.logRemoteOutput(DataScience.connectingToJupyterUri().format(connection.baseUrl));

// Create our session manager
this.sessionManager = (await this.sessionManagerFactory.create(connection)) as JupyterSessionManager;
}

public async createNotebook(
resource: Resource,
kernelConnection: KernelConnectionMetadata,
Expand Down Expand Up @@ -235,28 +207,22 @@ export class HostJupyterServer implements INotebookServer {
if (result === 10_000) {
traceError(`Session shutdown timed out.`);
}
this.sessionManager = undefined;
}

// After shutting down notebooks and session manager, kill the main process.
if (this.connection && this.connection) {
traceInfo('Shutdown server - dispose conn info');
this.connection.dispose(); // This should kill the process that's running
this.connection = undefined;
}
} catch (e) {
traceError(`Error during shutdown: `, e);
}
}

private waitForConnect(): Promise<IJupyterConnection | undefined> {
return this.connectPromise.promise;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Funky code now a thing of the past due to nullables being removed.

}

// Return a copy of the connection information that this server used to connect with
public getConnectionInfo(): IJupyterConnection | undefined {
public getConnectionInfo(): IJupyterConnection {
if (!this.connection) {
return undefined;
throw new Error('Not connected');
Copy link
Contributor Author

@DonJayamanne DonJayamanne Apr 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

THis is impossible, when the class is contructed, the first things that called is the connect method that initializes the property this.connection. Ideally we shoudl remove such code that initializes the class and then calls methods.
I've found this in a number of places and replaced them with factories, after that we dont end up with nullables (removing the nullabels makes the code simpler, e.g. in this case, IJupyterConnection can never be null in the any code path, but due to the way the class is written we don't know hence end up returning undefined and have to handle this case everywhere its used)

Copy link
Contributor Author

@DonJayamanne DonJayamanne Apr 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This class is now instantiated via a factory, hence connection is never nullable and the method getConnectionInfo is now a property

}

// Return a copy with a no-op for dispose
Expand Down
44 changes: 44 additions & 0 deletions src/kernels/jupyter/launcher/liveshare/hostJupyterServerFactory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

import { inject, injectable, named } from 'inversify';
import {} from 'underscore';
import { IWorkspaceService } from '../../../../platform/common/application/types';
import { STANDARD_OUTPUT_CHANNEL } from '../../../../platform/common/constants';
import { IAsyncDisposableRegistry, IDisposableRegistry, IOutputChannel } from '../../../../platform/common/types';
import { DataScience } from '../../../../platform/common/utils/localize';
import { traceInfo } from '../../../../platform/logging';
import { IJupyterConnection } from '../../../types';
import { JupyterSessionManager } from '../../session/jupyterSessionManager';
import { IJupyterSessionManagerFactory, INotebookServer, INotebookServerFactory } from '../../types';
import { HostJupyterServer } from './hostJupyterServer';

@injectable()
export class HostJupyterServerFactory implements INotebookServerFactory {
constructor(
@inject(IAsyncDisposableRegistry) private readonly asyncRegistry: IAsyncDisposableRegistry,
@inject(IJupyterSessionManagerFactory) private readonly sessionManagerFactory: IJupyterSessionManagerFactory,
@inject(IWorkspaceService) private readonly workspaceService: IWorkspaceService,
@inject(IOutputChannel) @named(STANDARD_OUTPUT_CHANNEL) private readonly jupyterOutputChannel: IOutputChannel,
@inject(IDisposableRegistry) private readonly disposables: IDisposableRegistry
) {}
public async createNotebookServer(connection: IJupyterConnection): Promise<INotebookServer> {
traceInfo(`Connecting server kernel ${connection.baseUrl}`);

// Indicate we have a new session on the output channel
if (!connection.localLaunch) {
this.jupyterOutputChannel.appendLine(DataScience.connectingToJupyterUri().format(connection.baseUrl));
}
// Create our session manager
const sessionManager = (await this.sessionManagerFactory.create(connection)) as JupyterSessionManager;
// Create a server tha t we will then attempt to connect to.
return new HostJupyterServer(
this.asyncRegistry,
this.workspaceService,
this.jupyterOutputChannel,
this.disposables,
connection,
sessionManager
);
}
}
16 changes: 4 additions & 12 deletions src/kernels/jupyter/launcher/liveshare/serverCache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { INotebookServerOptions, INotebookServer } from '../../types';

interface IServerData {
options: INotebookServerOptions;
promise: Promise<INotebookServer | undefined>;
promise: Promise<INotebookServer>;
resolved: boolean;
}

Expand All @@ -28,13 +28,10 @@ export class ServerCache implements IAsyncDisposable {
}

public async getOrCreate(
createFunction: (
options: INotebookServerOptions,
cancelToken: CancellationToken
) => Promise<INotebookServer | undefined>,
createFunction: (options: INotebookServerOptions, cancelToken: CancellationToken) => Promise<INotebookServer>,
options: INotebookServerOptions,
cancelToken: CancellationToken
): Promise<INotebookServer | undefined> {
): Promise<INotebookServer> {
const fixedOptions = await this.generateDefaultOptions(options);
const key = this.generateKey(fixedOptions);
let data: IServerData | undefined;
Expand All @@ -53,12 +50,7 @@ export class ServerCache implements IAsyncDisposable {
}

return data.promise
.then((server: INotebookServer | undefined) => {
if (!server) {
this.cache.delete(key);
return undefined;
}

.then((server: INotebookServer) => {
// Change the dispose on it so we
// can detach from the server when it goes away.
const oldDispose = server.dispose.bind(server);
Expand Down
Loading