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

Jump to bottom

Prevent duplicate kernel startup #876

Merged
merged 3 commits into from
Jun 20, 2017
Merged

Prevent duplicate kernel startup #876

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6a230f1
Prevent duplicate kernel startup, fixes #870
BenRussert Jun 16, 2017
f467c71
Merge branch 'master' into fix-kernelstart
BenRussert Jun 17, 2017
b84dfd2
Add tests for startingKernel
BenRussert Jun 18, 2017
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: 6 additions & 0 deletions lib/kernel-manager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,12 @@ class KernelManager {
onStarted: ?(kernel: ZMQKernel) => void
) {
const displayName = kernelSpec.display_name;

// if kernel startup already in progress don't start additional kernel
if (store.startingKernels.get(displayName)) return;

store.startKernel(displayName);

let currentPath = getEditorDirectory(store.editor);
let projectPath;

Expand Down
15 changes: 12 additions & 3 deletions lib/store/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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 😄

@observable grammar: ?atom$Grammar;

Expand All @@ -25,10 +26,18 @@ class Store {
return null;
}

@action
startKernel(kernelDisplayName: string) {
this.startingKernels.set(kernelDisplayName, true);
}

@action
newKernel(kernel: Kernel) {
const mappedLanguage = Config.getJson("languageMappings")[kernel.language];
this.runningKernels.set(mappedLanguage || kernel.language, kernel);
const mappedLanguage =
Config.getJson("languageMappings")[kernel.language] || kernel.language;
this.runningKernels.set(mappedLanguage, kernel);
// delete startingKernel since store.kernel now in place to prevent duplicate kernel
this.startingKernels.delete(kernel.kernelSpec.display_name);
}

@action
Expand Down
22 changes: 20 additions & 2 deletions spec/store/index-spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import store from "./../../lib/store";
describe("Store initialize", () => {
it("should correctly initialize store", () => {
expect(store.subscriptions instanceof CompositeDisposable).toBeTruthy();
expect(isObservableMap(store.startingKernels)).toBeTruthy();
expect(isObservableMap(store.runningKernels)).toBeTruthy();
expect(isObservable(store, "editor")).toBeTruthy();
expect(isObservable(store, "grammar")).toBeTruthy();
Expand All @@ -17,6 +18,7 @@ describe("Store initialize", () => {
describe("Store", () => {
beforeEach(() => {
store.subscriptions = new CompositeDisposable();
store.startingKernels = new Map();
store.runningKernels = new Map();
store.editor = null;
store.grammar = null;
Expand All @@ -37,6 +39,7 @@ describe("Store", () => {
const notCurrentKernel = {
kernelSpec: { language: "mock grammar" }
};

const runningKernels = {
"current kernel": currentKernel,
"not current kernel": notCurrentKernel
Expand Down Expand Up @@ -117,9 +120,24 @@ describe("Store", () => {
});
});

it("should add new kernel", () => {
const kernel = { language: "null grammar", foo: "bar" };
it("should add new kernel and reset starting kernel indicator", () => {
const kernelSpec = {
language: "null grammar",
display_name: "null grammar"
};
const kernel = {
language: "null grammar",
foo: "bar",
kernelSpec: kernelSpec
};
const { display_name } = kernelSpec;

store.startKernel(display_name);
expect(store.startingKernels.get(display_name)).toBeTruthy();

store.newKernel(kernel);
expect(store.startingKernels.get(display_name)).toBeUndefined();

expect(store.runningKernels.size).toBe(1);
expect(store.runningKernels.get("null grammar").language).toBe(
"null grammar"
Expand Down