-
Notifications
You must be signed in to change notification settings - Fork 334
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
Prevent duplicate kernel startup #876
Conversation
@@ -10,7 +10,8 @@ import type Kernel from "./../kernel"; | |||
|
|||
class Store { | |||
subscriptions = new CompositeDisposable(); | |||
@observable runningKernels = new Map(); | |||
@observable startingKernels: Map<string, boolean> = new Map(); | |||
@observable runningKernels: Map<string, Kernel> = new Map(); | |||
@observable editor = atom.workspace.getActiveTextEditor(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ideally we could do this without even adding startingKernels
, but we would have to somehow give store
the new kernel before the kernel is actually ready. I suppose we could promisify or something, but am eager to see what you all think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of having a promise or to just give store
a new kernel even if it's not fully connected yet. But this likely requires some refactorings that will be a lot easier once #858 is closed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I'll use the phrase "we can iterate" here 😄
This is ready for review! There haven't been any comments on this or #876, what do you think? @nikitakit I remembered that you had mentioned this issue in #780, do you have any feedback? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rgbkrk What do you think should we go with this?
It's not the most elegant solution but it works 😉
@@ -10,7 +10,8 @@ import type Kernel from "./../kernel"; | |||
|
|||
class Store { | |||
subscriptions = new CompositeDisposable(); | |||
@observable runningKernels = new Map(); | |||
@observable startingKernels: Map<string, boolean> = new Map(); | |||
@observable runningKernels: Map<string, Kernel> = new Map(); | |||
@observable editor = atom.workspace.getActiveTextEditor(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of having a promise or to just give store
a new kernel even if it's not fully connected yet. But this likely requires some refactorings that will be a lot easier once #858 is closed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go with it 🚀
🆒 |
Since we rely on
store.kernel
to indicate if a new kernel should be startedstore
needs to be aware af startup pretty much immediately to avoid duplicate kernels. See #870 for a wordier explanation.The idea here is to somehow fail silently instead of creating dup kernels that may not shut down properly. I would be grateful for feedback!
To Duplicate:
ctrl-enter
on 2 or more lines quickly