Skip to content

Commit

Permalink
fix bug starting jupyter on compute server
Browse files Browse the repository at this point in the history
compute server jupyter kernels relied on a subtle behavior of the
cursors table.
  • Loading branch information
williamstein committed Oct 22, 2024
1 parent 9ee3732 commit a578c14
Show file tree
Hide file tree
Showing 7 changed files with 38 additions and 20 deletions.
2 changes: 1 addition & 1 deletion src/packages/frontend/compute/manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ export class ComputeServersManager extends EventEmitter {

getComputeServers = () => {
const servers = {};
const cursors = this.sync_db.get_cursors().toJS();
const cursors = this.sync_db.get_cursors({ excludeSelf: 'never' }).toJS();
for (const client_id in cursors) {
const server = cursors[client_id];
servers[decodeUUIDtoNum(client_id)] = {
Expand Down
8 changes: 5 additions & 3 deletions src/packages/frontend/frame-editors/code-editor/actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1150,9 +1150,11 @@ export class Actions<
let cursors: Map<string, List<Map<string, any>>> = Map();
this._syncstring
.get_cursors({
excludeSelf: !redux
excludeSelf: redux
.getStore("account")
.getIn(["editor_settings", "show_my_other_cursors"]),
.getIn(["editor_settings", "show_my_other_cursors"])
? "heuristic"
: "always",
})
.forEach((info, account_id) => {
info.get("locs").forEach((loc) => {
Expand Down Expand Up @@ -1198,7 +1200,7 @@ export class Actions<
}
const omit_lines: SetMap = {};
const cursors = this._syncstring.get_cursors?.({
excludeSelf: false,
excludeSelf: "never",
maxAge: 3 * 60 * 1000,
}); // there are situations where get_cursors isn't defined (seen this).
if (cursors) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -562,7 +562,7 @@ export class JupyterEditorActions extends BaseActions<JupyterEditorState> {

gotoUser(account_id: string, frameId?: string) {
const cursors = this.jupyter_actions.syncdb
.get_cursors({ maxAge: 0 })
.get_cursors({ maxAge: 0, excludeSelf: "never" })
?.toJS();
if (cursors == null) return; // no info
const locs = cursors[account_id]?.locs;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1023,7 +1023,9 @@ export class Actions<T extends State = State> extends BaseActions<T | State> {
}

gotoUser(account_id: string, frameId?: string) {
const cursors = this._syncstring.get_cursors({ maxAge: 0 })?.toJS();
const cursors = this._syncstring
.get_cursors({ maxAge: 0, excludeSelf: "never" })
?.toJS();
if (cursors == null) return; // no info
const locs = cursors[account_id]?.locs;
for (const loc of locs) {
Expand Down
6 changes: 4 additions & 2 deletions src/packages/frontend/jupyter/browser-actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -326,9 +326,11 @@ export class JupyterActions extends JupyterActions0 {
) {
return;
}
const excludeSelf = !this.redux
const excludeSelf = this.redux
.getStore("account")
.getIn(["editor_settings", "show_my_other_cursors"]);
.getIn(["editor_settings", "show_my_other_cursors"])
? "heuristic"
: "always";
const cursors = this.syncdb.get_cursors({ excludeSelf });
const cells = this.cursor_manager.process(this.store.get("cells"), cursors);
if (cells != null) {
Expand Down
2 changes: 1 addition & 1 deletion src/packages/frontend/syncdoc.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -447,7 +447,7 @@ class SynchronizedDocument2 extends SynchronizedDocument
delete_trailing_whitespace: =>
cm = @focused_codemirror()
omit_lines = {}
@_syncstring.get_cursors()?.map (x, _) =>
@_syncstring.get_cursors(excludeSelf:'never')?.map (x, _) =>
x.get('locs')?.map (loc) =>
y = loc.get('y')
if y?
Expand Down
34 changes: 23 additions & 11 deletions src/packages/sync/editor/generic/sync-doc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -530,7 +530,7 @@ export class SyncDoc extends EventEmitter {
maxAge: COMPUTE_THRESH_MS,
// don't exclude self since getComputeServerId called from the compute
// server also to know if it is the chosen one.
excludeSelf: false,
excludeSelf: "never",
});
const dbg = this.dbg("getComputeServerId");
dbg("num cursors = ", cursors.size);
Expand All @@ -549,6 +549,7 @@ export class SyncDoc extends EventEmitter {
}
}
}

return isFinite(minId) ? minId : 0;
};

Expand Down Expand Up @@ -1884,13 +1885,21 @@ export class SyncDoc extends EventEmitter {

/* Returns *immutable* Map from account_id to list
of cursor positions, if cursors are enabled.
- excludeSelf: do not include our own cursor
- maxAge: only include cursors that have been updated with maxAge ms from now.
*/
public get_cursors = ({
get_cursors = ({
maxAge = 60 * 1000,
excludeSelf = true,
// excludeSelf:
// 'always' -- *always* exclude self
// 'never' -- never exclude self
// 'heuristic' -- exclude self is older than last set from here, e.g., useful on
// frontend so we don't see our own cursor unless more than one browser.
excludeSelf = "always",
}: {
maxAge?: number;
excludeSelf?: boolean;
excludeSelf?: "always" | "never" | "heuristic";
} = {}): Map<string, any> => {
this.assert_not_closed("get_cursors");
if (!this.cursors) {
Expand All @@ -1901,12 +1910,15 @@ export class SyncDoc extends EventEmitter {
}
const account_id: string = this.client_id();
let map = this.cursor_map;
if (
(excludeSelf && map.has(account_id)) ||
this.cursor_last_time >=
(map.getIn([account_id, "time"], new Date(0)) as Date)
) {
map = map.delete(account_id);
if (map.has(account_id) && excludeSelf != "never") {
if (
excludeSelf == "always" ||
(excludeSelf == "heuristic" &&
this.cursor_last_time >=
(map.getIn([account_id, "time"], new Date(0)) as Date))
) {
map = map.delete(account_id);
}
}
// Remove any old cursors, where "old" is by default more than maxAge old.
const now = Date.now();
Expand Down Expand Up @@ -1939,7 +1951,7 @@ export class SyncDoc extends EventEmitter {
/* Set settings map. Used for custom configuration just for
this one file, e.g., overloading the spell checker language.
*/
public set_settings = async (obj): Promise<void> => {
set_settings = async (obj): Promise<void> => {
this.assert_is_ready("set_settings");
await this.set_syncstring_table({
settings: obj,
Expand Down

0 comments on commit a578c14

Please sign in to comment.