Restart for raw sessions + rawSession now owns process lifetime #11230
Conversation
| public abstract async changeKernel(kernel: IJupyterKernelSpec | LiveKernelModel, timeoutMS: number): Promise<void>; | ||
| public abstract async waitForIdle(timeout: number): Promise<void>; | ||
|
|
||
| public async shutdown(): Promise<void> { |
There was a problem hiding this comment.
Most of the big chunks of changes in this file are not new changes, but refactors. Previous to this change I had restart and shutdown just implemented in the raw and jupyter session classes. Now they have moved into the base class so that they can share code. So shutdown and restart code was pulled from the jupyter specific class into the base class and changes were made to support that.
| @@ -228,6 +330,68 @@ export abstract class BaseJupyterSession implements IJupyterSession { | |||
| } | |||
| } | |||
|
|
|||
| // Sub classes need to implement their own restarting specific code | |||
| protected abstract startRestartSession(): void; | |||
There was a problem hiding this comment.
The restart code above is common and unchanged from previous jupyter code, but there are specific start and restart functions for the jupyter / raw subclasses.
| protected async shutdownSession( | ||
| session: ISession | undefined, | ||
| statusHandler: Slot<ISession, Kernel.Status> | undefined | ||
| ): Promise<void> { |
There was a problem hiding this comment.
Previous JupyterSession code for shutting down a session
There was a problem hiding this comment.
Thanks for comment, was trying to figure out what waas old and new.
This is one of those cases where I feel splitting PR into two helps, one for refactoring and other for new changes. This way the refactoring code requires less review, as we're merely moving stuff around.
Also makes for smaller PRs..
There was a problem hiding this comment.
Yeah, I did that with the notebook provider refactor and it was really helpful. But I didn't here, will try to make sure that I do that in the future as much as I can. 👍 👍 For smaller PRs as well.
| @@ -28,9 +28,9 @@ export interface IKernelConnection { | |||
|
|
|||
| export interface IKernelProcess extends IDisposable { | |||
| process: ChildProcess | undefined; | |||
| readonly connection: Readonly<IKernelConnection> | undefined; | |||
| readonly connection: Readonly<IKernelConnection>; | |||
There was a problem hiding this comment.
I changed the code in a previous checkin to pass the connection and spec in directly to kernelProcess constructor so they are guaranteed, but didn't pull the undefined off of the type.
| @@ -24,55 +26,33 @@ It's responsible for translating our IJupyterSession interface into the | |||
| jupyterlabs interface as well as starting up and connecting to a raw session | |||
| */ | |||
| export class RawJupyterSession extends BaseJupyterSession { | |||
| private currentKernelProcess: IKernelProcess | undefined; | |||
There was a problem hiding this comment.
I really didn't like keeping the process off here separate from the session. Was ok with one session, but when you added in a restart session it was getting ugly to juggle. So I made a change to push the process into the lower level RawSession. This helps with both not having to track it here when I restart session or change kernels, but now the shared shutdownSession code between the jupyter and raw session can both just call shutdown and the RawSession will dispose the process at that point.
| @@ -43,13 +47,21 @@ suite('Data Science - RawJupyterSession', () => { | |||
| // Set up a fake kernel process for the launcher to return | |||
| processExitEvent = new EventEmitter<number | null>(); | |||
| kernelProcess = createTypeMoq<IKernelProcess>('kernel process'); | |||
| kernelProcess.setup((kp) => kp.kernelSpec).returns(() => 'testspec' as any); | |||
| kernelProcess | |||
There was a problem hiding this comment.
Since I have an "in" check to see if my spec is not a live model I needed this as an object.
IanMatthewHuff
requested review from
DonJayamanne,
DavidKutu,
greazer and
rchiodo
|
@rchiodo @DonJayamanne @DavidKutu . Would someone mind taking a gander at this PR? Would like to make changes before our dogfooding if I can. |
|
Kudos, SonarCloud Quality Gate passed!
|
For #10767
package-lock.jsonhas been regenerated by runningnpm install(if dependencies have changed).