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.

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

🗑 feat/dispose cycles #614

Merged
merged 7 commits into from
Apr 19, 2023
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
1 change: 0 additions & 1 deletion packages/core/src/manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,6 @@ export class ThebeManager extends KernelWidgetManager {
}

async display_view(msg: any, view: any, options: any): Promise<Widget> {
console.log('widget display_view', { view, options });
if (options.el) {
LuminoWidget.Widget.attach(view.luminoWidget, options.el);
}
Expand Down
13 changes: 3 additions & 10 deletions packages/core/src/passive.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,24 +44,17 @@ function stripWidgets(outputs: nbformat.IOutput[], hideWidgets?: boolean) {
class PassiveCellRenderer implements IPassiveCell {
readonly id: string;
readonly rendermime: IRenderMimeRegistry;
readonly hideWidgets: boolean;
protected model: OutputAreaModel;
protected area: OutputArea;

constructor(
id: string,
rendermime?: IRenderMimeRegistry,
mathjax?: MathjaxOptions,
hideWidgets = false,
) {
constructor(id: string, rendermime?: IRenderMimeRegistry, mathjax?: MathjaxOptions) {
this.id = id;
this.rendermime = rendermime ?? getRenderMimeRegistry(mathjax ?? makeMathjaxOptions());
this.model = new OutputAreaModel({ trusted: true });
this.area = new OutputArea({
model: this.model,
rendermime: this.rendermime,
});
this.hideWidgets = hideWidgets;
}

/**
Expand Down Expand Up @@ -151,8 +144,8 @@ class PassiveCellRenderer implements IPassiveCell {
* @param outputs - serialised jupyter outputs
* @returns
*/
render(outputs: nbformat.IOutput[]) {
this.model.fromJSON(stripWidgets(outputs, this.hideWidgets));
render(outputs: nbformat.IOutput[], hideWidgets?: boolean) {
this.model.fromJSON(stripWidgets(outputs, hideWidgets));
}
}

Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,8 @@ class ThebeServer implements ServerRuntime, ServerRestAPI {

dispose() {
if (this._isDisposed) return;
this.serviceManager?.dispose();
this.sessionManager?.dispose();
if (!this.serviceManager?.isDisposed) this.serviceManager?.dispose();
if (!this.sessionManager?.isDisposed) this.sessionManager?.dispose();

// Implementing the flag at this level as
this._isDisposed = true;
Expand Down
5 changes: 3 additions & 2 deletions packages/core/src/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,16 +59,17 @@ class ThebeSession {
}

async shutdown() {
if (this.connection.isDisposed) return;
await this.connection.shutdown();
this.events.triggerStatus({
status: SessionStatusEvent.shutdown,
message: `session ${this.name}`,
});
return this.connection.dispose();
this.dispose();
}

dispose() {
this.connection.dispose();
if (!this.connection.isDisposed) this.connection.dispose();
}
}

Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ export interface IPassiveCell {
setOutputText(text: string): void;
clear(): void;
clearOnError(error?: any): void;
render(outputs: IOutput[]): void;
render(outputs: IOutput[], hideWidgets?: boolean): void;
}

export interface IThebeCell extends IPassiveCell {
Expand Down
19 changes: 14 additions & 5 deletions packages/react/src/ThebeServerProvider.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ export const ThebeServerContext = React.createContext<
connecting: boolean;
ready: boolean;
connect: () => void;
disconnect: () => Promise<void>;
}
| undefined
>(undefined);
Expand All @@ -42,6 +43,7 @@ export function ThebeServerProvider({
const { core } = useThebeCore();
const [doConnect, setDoConnect] = useState(connect);
const [connecting, setConnecting] = useState<boolean>(false);
const [server, setServer] = useState<ThebeServer | undefined>();
const [ready, setReady] = useState<boolean>(false);

// create a valid configuration, either using the one supplied
Expand All @@ -52,11 +54,9 @@ export function ThebeServerProvider({
[core, options],
);

// because core is async, server is still maybe undefined, but can be created before
// the connection function is kicked off
const server = useMemo(() => {
useEffect(() => {
if (!core || !thebeConfig) return;
return new core.ThebeServer(thebeConfig);
setServer(new core.ThebeServer(thebeConfig));
}, [core, thebeConfig]);

// Once the core is loaded, connect to a server
Expand All @@ -82,6 +82,14 @@ export function ThebeServerProvider({
connecting,
ready: (server?.isReady ?? false) && ready, // TODO server status may change, affecting readiness
connect: () => setDoConnect(true),
disconnect: async () => {
if (core && thebeConfig && server) {
server.dispose();
setServer(new core.ThebeServer(thebeConfig));
}
setReady(false);
setDoConnect(false);
},
}}
>
{children}
Expand Down Expand Up @@ -124,7 +132,7 @@ export function useThebeServer() {
if (serverContext === undefined) {
throw new Error('useThebeServer must be used inside a ThebeServerProvider');
}
const { config, events, server, connecting, ready, connect } = serverContext;
const { config, events, server, connecting, ready, connect, disconnect } = serverContext;

const { core } = useThebeCore();
const [error, setError] = useState<string | undefined>(); // TODO how to handle errors better via the provider
Expand Down Expand Up @@ -168,6 +176,7 @@ export function useThebeServer() {
ready,
error,
connect,
disconnect,
subscribe,
unsubAll,
};
Expand Down
63 changes: 41 additions & 22 deletions packages/react/src/ThebeSessionProvider.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ interface ThebeSessionContextData {
starting: boolean;
ready: boolean;
error?: string;
start: () => void;
start: () => Promise<void>;
shutdown: () => Promise<void>;
}

export const ThebeSessionContext = React.createContext<ThebeSessionContextData | undefined>(
Expand All @@ -21,44 +22,53 @@ export function ThebeSessionProvider({
shutdownOnUnmount = false,
children,
}: React.PropsWithChildren<{
start: boolean;
start?: boolean;
name?: string;
shutdownOnUnmount?: boolean;
}>) {
const { config, server, ready: serverReady } = useThebeServer();

const [doStart, setDoStart] = useState(start);
const [starting, setStarting] = useState(false);
const [sessionName, setSessionName] = useState(name);
const [session, setSession] = useState<ThebeSession | undefined>();
const [ready, setReady] = useState(false);
const [error, setError] = useState<string | undefined>();

/// Once server connection is open, start a session
useEffect(() => {
if (!doStart || !server || !serverReady) return;
const startSession = () => {
setStarting(true);
server
.startNewSession({ ...config?.kernels, name: sessionName, path: sessionName })
.then((sesh) => {
?.startNewSession({ ...config?.kernels, name, path: name })
.then((sesh: ThebeSession | null) => {
setStarting(false);
if (sesh != null) return setSession(sesh);
server.getKernelSpecs().then((specs) => {
setError(
`Could not start a session - available kernels: ${Object.keys(specs.kernelspecs)}`,
);
});
if (sesh == null) {
server?.getKernelSpecs().then((specs) => {
setError(
`Could not start a session - available kernels: ${Object.keys(specs.kernelspecs)}`,
);
});
return;
}
setSession(sesh);
setReady(true); // not this could use the thebe event mechanism
});
}, [doStart, server, serverReady]);
};

/// Once server connection is open, auto start a session if start prop is true
useEffect(() => {
if (!server || !serverReady || !starting || !start) return;
startSession();
}, [start, starting, server, serverReady]);

// shutdown session on navigate away
useEffect(() => {
return () => {
if (shutdownOnUnmount) session?.shutdown();
if (shutdownOnUnmount) {
session?.shutdown().then(() => {
setReady(false);
});
}
};
}, [session]);

const ready = !!session && session.kernel != null;

return (
<ThebeSessionContext.Provider
value={{
Expand All @@ -67,9 +77,18 @@ export function ThebeSessionProvider({
ready,
session,
// eslint-disable-next-line @typescript-eslint/no-shadow
start: (name?: string) => {
if (name) setSessionName(name);
setDoStart(true);
start: async () => {
if (!!session && ready) {
await session.restart();
} else {
startSession();
}
},
shutdown: async () => {
if (session) {
await session.shutdown();
setReady(false);
}
},
error,
}}
Expand Down