From f532c1868fbe3f776165a00073b1a2767d527e3a Mon Sep 17 00:00:00 2001 From: Fons van der Plas Date: Sun, 11 Aug 2024 12:22:03 +0200 Subject: [PATCH 01/11] maybe fix? --- frontend/components/Editor.js | 13 ++++++++++ frontend/imports/immer.js | 14 +++++++++++ src/evaluation/Run.jl | 2 +- src/evaluation/RunBonds.jl | 2 +- src/evaluation/Throttled.jl | 23 +++++++++++++----- src/webserver/Dynamic.jl | 45 +++++++++++++++++++++++++---------- 6 files changed, 79 insertions(+), 20 deletions(-) diff --git a/frontend/components/Editor.js b/frontend/components/Editor.js index b1a705eedd..1081c761ce 100644 --- a/frontend/components/Editor.js +++ b/frontend/components/Editor.js @@ -699,6 +699,10 @@ export class Editor extends Component { // throw new Error(`Error: [Immer] minified error nr: 15 '${patches?.[0]?.path?.join("/")}' .`) // } + window.immer = immer + window.applyPatches = applyPatches + window.produceWithPatches = produceWithPatches + if (get_reverse_patches) { ;[new_notebook, _copy_of_patches, reverse_of_patches] = produceWithPatches(old_state ?? state.notebook, (state) => { applyPatches(state, patches) @@ -711,6 +715,7 @@ export class Editor extends Component { /** @type {String} Example: `"a.b[2].c"` */ const failing_path = String(exception).match(".*'(.*)'.*")?.[1].replace(/\//gi, ".") ?? exception const path_value = _.get(this.state.notebook, failing_path, "Not Found") + console.log(String(exception).match(".*'(.*)'.*")?.[1].replace(/\//gi, ".") ?? exception, failing_path, typeof failing_path) const ignore = should_ignore_patch_error(failing_path) @@ -725,10 +730,18 @@ patch: ${JSON.stringify( null, 1 )} +all patches: ${JSON.stringify(patches, null, 1)} #######################**************************########################`, exception ) + let parts = failing_path.split(".") + for (let i = 0; i < parts.length; i++) { + let path = parts.slice(0, i).join(".") + let value = _.get(this.state.notebook, path, "Not Found") + console.log(path, value) + } + if (ignore) { console.info("Safe to ignore this patch failure...") } else if (this.state.connected) { diff --git a/frontend/imports/immer.js b/frontend/imports/immer.js index aec3b510ac..66af43e3cd 100644 --- a/frontend/imports/immer.js +++ b/frontend/imports/immer.js @@ -11,3 +11,17 @@ enablePatches() // The solution is to tell immer not to create immutable objects setAutoFreeze(false) + +// // @ts-nocheck +// import { produce as immer, produceWithPatches, applyPatches, enablePatches, setAutoFreeze } from "https://cdn.jsdelivr.net/npm/immer@10.1.1/dist/immer.mjs" + +// export { applyPatches, produceWithPatches } +// export default immer + +// enablePatches() + +// // we have some Editor.setState functions that use immer, so Editor.this.state becomes an "immer immutable frozen object". But we also have some Editor.setState functions that don't use immer, and they try to _mutate_ Editor.this.state. This gives errors like https://github.com/immerjs/immer/issues/576 + +// // The solution is to tell immer not to create immutable objects + +// setAutoFreeze(false) diff --git a/src/evaluation/Run.jl b/src/evaluation/Run.jl index 81be9f736b..54c2a9591e 100644 --- a/src/evaluation/Run.jl +++ b/src/evaluation/Run.jl @@ -123,7 +123,7 @@ function run_reactive_core!( # Send intermediate updates to the clients at most 20 times / second during a reactive run. (The effective speed of a slider is still unbounded, because the last update is not throttled.) # flush_send_notebook_changes_throttled, - send_notebook_changes_throttled = Throttled.throttled(1.0 / 20; runtime_multiplier=2.0) do + send_notebook_changes_throttled = Throttled.throttled(1.0 / 20; runtime_multiplier=2.0, run_threaded=false) do # We will do a state sync now, so that means that we can delay the status_tree state sync loop, see https://github.com/fonsp/Pluto.jl/issues/2978 Throttled.force_throttle_without_run(notebook.status_tree.update_listener_ref[]) # State sync: diff --git a/src/evaluation/RunBonds.jl b/src/evaluation/RunBonds.jl index 7ec4e191b6..a51ee0ac11 100644 --- a/src/evaluation/RunBonds.jl +++ b/src/evaluation/RunBonds.jl @@ -33,7 +33,7 @@ function set_bond_values_reactive(; )::Vector{Symbol} if isempty(syms_to_set) || !will_run_code(notebook) - send_notebook_changes!(ClientRequest(; session, notebook, initiator)) + send_notebook_changes!(ClientRequest(; session, notebook, initiator); should_lock=false) return TopologicalOrder(notebook.topology, Cell[], Dict{Cell,PlutoDependencyExplorer.ReactivityError}()) end diff --git a/src/evaluation/Throttled.jl b/src/evaluation/Throttled.jl index aac2f2cb13..e6f853d538 100644 --- a/src/evaluation/Throttled.jl +++ b/src/evaluation/Throttled.jl @@ -11,6 +11,7 @@ struct ThrottledFunction iscoolnow::Ref{Bool} run_later::Ref{Bool} last_runtime::Ref{Float64} + run_threaded::Bool end "Run the function now" @@ -22,13 +23,24 @@ function Base.flush(tf::ThrottledFunction) end end +function _trigger(tf::ThrottledFunction) + if tf.run_threaded + Threads.@spawn begin + flush(tf) + schedule(tf) + end + else + flush(tf) + schedule(tf) + end +end + "Start the cooldown period. If at the end, a run_later[] is set, then we run the function and schedule the next cooldown period." function schedule(tf::ThrottledFunction) # if the last runtime was quite long, increase the sleep period to match. Timer(tf.timeout + tf.last_runtime[] * tf.runtime_multiplier) do _t if tf.run_later[] - flush(tf) - schedule(tf) + _trigger(tf) else tf.iscoolnow[] = true end @@ -38,8 +50,7 @@ end function (tf::ThrottledFunction)() if tf.iscoolnow[] tf.iscoolnow[] = false - flush(tf) - schedule(tf) + _trigger(tf) else tf.run_later[] = true end @@ -63,13 +74,13 @@ This throttle is 'leading' and has some other properties that are specifically d Inspired by FluxML See: https://github.com/FluxML/Flux.jl/blob/8afedcd6723112ff611555e350a8c84f4e1ad686/src/utils.jl#L662 """ -function throttled(f::Function, timeout::Real; runtime_multiplier::Float64=0.0) +function throttled(f::Function, timeout::Real; runtime_multiplier::Float64=0.0, run_threaded::Bool=false) tlock = ReentrantLock() iscoolnow = Ref(false) run_later = Ref(false) last_runtime = Ref(0.0) - tf = ThrottledFunction(f, timeout, runtime_multiplier, tlock, iscoolnow, run_later, last_runtime) + tf = ThrottledFunction(f, timeout, runtime_multiplier, tlock, iscoolnow, run_later, last_runtime, run_threaded) # we initialize hot, and start the cooldown period immediately schedule(tf) diff --git a/src/webserver/Dynamic.jl b/src/webserver/Dynamic.jl index 76548c427d..5fa808f081 100644 --- a/src/webserver/Dynamic.jl +++ b/src/webserver/Dynamic.jl @@ -162,15 +162,20 @@ end For each connected client, we keep a copy of their current state. This way we know exactly which updates to send when the server-side state changes. """ const current_state_for_clients = WeakKeyDict{ClientSession,Any}() -const current_state_for_clients_lock = ReentrantLock() +const current_state_for_clients_lock = Base.Semaphore(1) + +dontacquire(f::Function, z) = f() """ Update the local state of all clients connected to this notebook. """ -function send_notebook_changes!(🙋::ClientRequest; commentary::Any=nothing, skip_send::Bool=false) +function send_notebook_changes!(🙋::ClientRequest; commentary::Any=nothing, skip_send::Bool=false, should_lock::Bool=true) outbox = Set{Tuple{ClientSession,UpdateMessage}}() - lock(current_state_for_clients_lock) do + @debug "y1" should_lock + + (should_lock ? Base.acquire : dontacquire)(current_state_for_clients_lock) do + @debug "y2" should_lock notebook_dict = notebook_to_js(🙋.notebook) for (_, client) in 🙋.session.connected_clients if client.connected_notebook !== nothing && client.connected_notebook.notebook_id == 🙋.notebook.notebook_id @@ -184,6 +189,8 @@ function send_notebook_changes!(🙋::ClientRequest; commentary::Any=nothing, sk if !skip_send && (!isempty(patches) || is_response) response = Dict( + :before_status => current_dict isa Symbol ? :empty : current_dict["status_tree"], + :after_status => notebook_dict["status_tree"], :patches => patches_as_dicts, :response => is_response ? commentary : nothing ) @@ -191,12 +198,12 @@ function send_notebook_changes!(🙋::ClientRequest; commentary::Any=nothing, sk end end end + + for (client, msg) in outbox + putclientupdates!(client, msg) + end + try_event_call(🙋.session, FileEditEvent(🙋.notebook)) end - - for (client, msg) in outbox - putclientupdates!(client, msg) - end - try_event_call(🙋.session, FileEditEvent(🙋.notebook)) end "Like `deepcopy`, but anything other than `Dict` gets a shallow (reference) copy." @@ -289,22 +296,29 @@ const effects_of_changed_state = Dict( responses[:update_notebook] = function response_update_notebook(🙋::ClientRequest) require_notebook(🙋) try + @debug "x1" + Base.acquire(current_state_for_clients_lock) + @debug "x2" notebook = 🙋.notebook patches = (Base.convert(Firebasey.JSONPatch, update) for update in 🙋.body["updates"]) if length(patches) == 0 - send_notebook_changes!(🙋) + @debug "x2zzzzz" + send_notebook_changes!(🙋; should_lock=false) + @debug "x2zzzzz 777" return nothing end if !haskey(current_state_for_clients, 🙋.initiator.client) throw(ErrorException("Updating without having a first version of the notebook??")) end + @debug "x2a" # TODO Immutable ?? for patch in patches Firebasey.applypatch!(current_state_for_clients[🙋.initiator.client], patch) end + @debug "x2b" changes = Set{Changed}() @@ -319,6 +333,7 @@ responses[:update_notebook] = function response_update_notebook(🙋::ClientRequ union!(changes, current_changes) end + @debug "x2c" # We put a flag to check whether any patch changes the skip_as_script metadata. This is to eventually trigger a notebook updated if no reactive_run is part of this update skip_as_script_changed = any(patches) do patch @@ -331,6 +346,7 @@ responses[:update_notebook] = function response_update_notebook(🙋::ClientRequ end end + @debug "x2d" # If CodeChanged ∈ changes, then the client will also send a request like run_multiple_cells, which will trigger a file save _before_ running the cells. # In the future, we should get rid of that request, and save the file here. For now, we don't save the file here, to prevent unnecessary file IO. # (You can put a log in save_notebook to track how often the file is saved) @@ -339,8 +355,9 @@ responses[:update_notebook] = function response_update_notebook(🙋::ClientRequ # If skip_as_script has changed but no cell run is happening we want to update the notebook dependency here before saving the file update_skipped_cells_dependency!(notebook) end - save_notebook(🙋.session, notebook) + save_notebook(🙋.session, notebook) end + @debug "x2e" let bond_changes = filter(x -> x isa BondChanged, changes) bound_sym_names = Symbol[x.bond_name for x in bond_changes] @@ -355,7 +372,8 @@ responses[:update_notebook] = function response_update_notebook(🙋::ClientRequ ) end - send_notebook_changes!(🙋; commentary=Dict(:update_went_well => :👍)) + send_notebook_changes!(🙋; commentary=Dict(:update_went_well => :👍), should_lock=false) + @debug "x2g" catch ex @error "Update notebook failed" 🙋.body["updates"] exception=(ex, stacktrace(catch_backtrace())) response = Dict( @@ -363,7 +381,10 @@ responses[:update_notebook] = function response_update_notebook(🙋::ClientRequ :why_not => sprint(showerror, ex), :should_i_tell_the_user => ex isa SessionActions.UserError, ) - send_notebook_changes!(🙋; commentary=response) + send_notebook_changes!(🙋; commentary=response, should_lock=false) + finally + @debug "x3" + Base.release(current_state_for_clients_lock) end end From 0f45c1964a0ed9016e9354651377dae8e68eb85d Mon Sep 17 00:00:00 2001 From: Fons van der Plas Date: Sun, 11 Aug 2024 12:22:36 +0200 Subject: [PATCH 02/11] remove logs --- src/webserver/Dynamic.jl | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/src/webserver/Dynamic.jl b/src/webserver/Dynamic.jl index 5fa808f081..a1e0c0ca4f 100644 --- a/src/webserver/Dynamic.jl +++ b/src/webserver/Dynamic.jl @@ -172,10 +172,7 @@ Update the local state of all clients connected to this notebook. function send_notebook_changes!(🙋::ClientRequest; commentary::Any=nothing, skip_send::Bool=false, should_lock::Bool=true) outbox = Set{Tuple{ClientSession,UpdateMessage}}() - @debug "y1" should_lock - (should_lock ? Base.acquire : dontacquire)(current_state_for_clients_lock) do - @debug "y2" should_lock notebook_dict = notebook_to_js(🙋.notebook) for (_, client) in 🙋.session.connected_clients if client.connected_notebook !== nothing && client.connected_notebook.notebook_id == 🙋.notebook.notebook_id @@ -296,9 +293,7 @@ const effects_of_changed_state = Dict( responses[:update_notebook] = function response_update_notebook(🙋::ClientRequest) require_notebook(🙋) try - @debug "x1" Base.acquire(current_state_for_clients_lock) - @debug "x2" notebook = 🙋.notebook patches = (Base.convert(Firebasey.JSONPatch, update) for update in 🙋.body["updates"]) @@ -312,13 +307,11 @@ responses[:update_notebook] = function response_update_notebook(🙋::ClientRequ if !haskey(current_state_for_clients, 🙋.initiator.client) throw(ErrorException("Updating without having a first version of the notebook??")) end - @debug "x2a" # TODO Immutable ?? for patch in patches Firebasey.applypatch!(current_state_for_clients[🙋.initiator.client], patch) end - @debug "x2b" changes = Set{Changed}() @@ -333,7 +326,6 @@ responses[:update_notebook] = function response_update_notebook(🙋::ClientRequ union!(changes, current_changes) end - @debug "x2c" # We put a flag to check whether any patch changes the skip_as_script metadata. This is to eventually trigger a notebook updated if no reactive_run is part of this update skip_as_script_changed = any(patches) do patch @@ -346,7 +338,6 @@ responses[:update_notebook] = function response_update_notebook(🙋::ClientRequ end end - @debug "x2d" # If CodeChanged ∈ changes, then the client will also send a request like run_multiple_cells, which will trigger a file save _before_ running the cells. # In the future, we should get rid of that request, and save the file here. For now, we don't save the file here, to prevent unnecessary file IO. # (You can put a log in save_notebook to track how often the file is saved) @@ -357,7 +348,6 @@ responses[:update_notebook] = function response_update_notebook(🙋::ClientRequ end save_notebook(🙋.session, notebook) end - @debug "x2e" let bond_changes = filter(x -> x isa BondChanged, changes) bound_sym_names = Symbol[x.bond_name for x in bond_changes] @@ -373,7 +363,6 @@ responses[:update_notebook] = function response_update_notebook(🙋::ClientRequ end send_notebook_changes!(🙋; commentary=Dict(:update_went_well => :👍), should_lock=false) - @debug "x2g" catch ex @error "Update notebook failed" 🙋.body["updates"] exception=(ex, stacktrace(catch_backtrace())) response = Dict( @@ -383,7 +372,6 @@ responses[:update_notebook] = function response_update_notebook(🙋::ClientRequ ) send_notebook_changes!(🙋; commentary=response, should_lock=false) finally - @debug "x3" Base.release(current_state_for_clients_lock) end end From e34fdf20c7db476a0086fa416ee550c6da94b742 Mon Sep 17 00:00:00 2001 From: Fons van der Plas Date: Sun, 11 Aug 2024 13:04:35 +0200 Subject: [PATCH 03/11] Update Dynamic.jl --- src/webserver/Dynamic.jl | 53 ++++++++++++++++++++++++++++++++++------ 1 file changed, 46 insertions(+), 7 deletions(-) diff --git a/src/webserver/Dynamic.jl b/src/webserver/Dynamic.jl index a1e0c0ca4f..1feea48519 100644 --- a/src/webserver/Dynamic.jl +++ b/src/webserver/Dynamic.jl @@ -166,6 +166,19 @@ const current_state_for_clients_lock = Base.Semaphore(1) dontacquire(f::Function, z) = f() +get_e_1(x) = try + join(x["status_tree"]["subtasks"]["run"]["subtasks"]["evaluate"]["subtasks"] |> keys, ", ") +catch + "empty" +end + +interesting_patches(patches) = filter(patch -> ["status_tree", "subtasks", "run", "subtasks", "evaluate", "subtasks"] ⊆ patch.path, patches) + + + +const update_counter = Ref(0) + + """ Update the local state of all clients connected to this notebook. """ @@ -177,17 +190,28 @@ function send_notebook_changes!(🙋::ClientRequest; commentary::Any=nothing, sk for (_, client) in 🙋.session.connected_clients if client.connected_notebook !== nothing && client.connected_notebook.notebook_id == 🙋.notebook.notebook_id current_dict = get(current_state_for_clients, client, :empty) + + counter = update_counter[] += 1 + patches = Firebasey.diff(current_dict, notebook_dict) patches_as_dicts = Firebasey._convert(Vector{Dict}, patches) current_state_for_clients[client] = deep_enough_copy(notebook_dict) + let ip = interesting_patches(patches) + if !isempty(ip) + @debug "snc" get_e_1(current_dict) get_e_1(notebook_dict) ip counter + end + end + + # Make sure we do send a confirmation to the client who made the request, even without changes is_response = 🙋.initiator !== nothing && client == 🙋.initiator.client if !skip_send && (!isempty(patches) || is_response) response = Dict( - :before_status => current_dict isa Symbol ? :empty : current_dict["status_tree"], - :after_status => notebook_dict["status_tree"], + :counter => counter, + :before_status => get_e_1(current_dict), + :after_status => get_e_1(notebook_dict), :patches => patches_as_dicts, :response => is_response ? commentary : nothing ) @@ -298,9 +322,7 @@ responses[:update_notebook] = function response_update_notebook(🙋::ClientRequ patches = (Base.convert(Firebasey.JSONPatch, update) for update in 🙋.body["updates"]) if length(patches) == 0 - @debug "x2zzzzz" send_notebook_changes!(🙋; should_lock=false) - @debug "x2zzzzz 777" return nothing end @@ -309,9 +331,12 @@ responses[:update_notebook] = function response_update_notebook(🙋::ClientRequ end # TODO Immutable ?? + # b = get_e_1(current_state_for_clients[🙋.initiator.client]) for patch in patches Firebasey.applypatch!(current_state_for_clients[🙋.initiator.client], patch) end + # a = get_e_1(current_state_for_clients[🙋.initiator.client]) + # @debug "un" b a changes = Set{Changed}() @@ -437,10 +462,24 @@ responses[:run_multiple_cells] = function response_run_multiple_cells(🙋::Clie if will_run_code(🙋.notebook) foreach(c -> c.queued = true, cells) - # run send_notebook_changes! without actually sending it, to update current_state_for_clients for our client with c.queued = true. + # update current_state_for_clients for our client with c.queued = true. # later, during update_save_run!, the cell will actually run, eventually setting c.queued = false again, which will be sent to the client through a patch update. - # We *need* to send *something* to the client, because of https://github.com/fonsp/Pluto.jl/pull/1892, but we also don't want to send unnecessary updates. We can skip sending this update, because update_save_run! will trigger a send_notebook_changes! very very soon. - send_notebook_changes!(🙋; skip_send=true) + # This guarantees that something will be sent. + # We *need* to send *something* to the client, because of https://github.com/fonsp/Pluto.jl/pull/1892, but we also don't want to send unnecessary updates. We can skip sending this update with send_notebook_changes!, because update_save_run! will trigger a send_notebook_changes! very very soon. + # send_notebook_changes!(🙋; skip_send=true) + + for (_, client) in 🙋.session.connected_clients + if client.connected_notebook !== nothing && client.connected_notebook.notebook_id == 🙋.notebook.notebook_id + if haskey(current_state_for_clients, client) + results = current_state_for_clients[client]["cell_results"] + for c in cells + if haskey(results, cell.cell_id) + results[cell.cell_id]["queued"] = true + end + end + end + end + end end function on_auto_solve_multiple_defs(disabled_cells_dict) From 6b35239f5b29609013dc72333bce1885dd1ab7cd Mon Sep 17 00:00:00 2001 From: Fons van der Plas Date: Mon, 12 Aug 2024 15:11:58 +0200 Subject: [PATCH 04/11] Discard changes to src/evaluation/Throttled.jl --- src/evaluation/Throttled.jl | 23 ++++++----------------- 1 file changed, 6 insertions(+), 17 deletions(-) diff --git a/src/evaluation/Throttled.jl b/src/evaluation/Throttled.jl index e6f853d538..aac2f2cb13 100644 --- a/src/evaluation/Throttled.jl +++ b/src/evaluation/Throttled.jl @@ -11,7 +11,6 @@ struct ThrottledFunction iscoolnow::Ref{Bool} run_later::Ref{Bool} last_runtime::Ref{Float64} - run_threaded::Bool end "Run the function now" @@ -23,24 +22,13 @@ function Base.flush(tf::ThrottledFunction) end end -function _trigger(tf::ThrottledFunction) - if tf.run_threaded - Threads.@spawn begin - flush(tf) - schedule(tf) - end - else - flush(tf) - schedule(tf) - end -end - "Start the cooldown period. If at the end, a run_later[] is set, then we run the function and schedule the next cooldown period." function schedule(tf::ThrottledFunction) # if the last runtime was quite long, increase the sleep period to match. Timer(tf.timeout + tf.last_runtime[] * tf.runtime_multiplier) do _t if tf.run_later[] - _trigger(tf) + flush(tf) + schedule(tf) else tf.iscoolnow[] = true end @@ -50,7 +38,8 @@ end function (tf::ThrottledFunction)() if tf.iscoolnow[] tf.iscoolnow[] = false - _trigger(tf) + flush(tf) + schedule(tf) else tf.run_later[] = true end @@ -74,13 +63,13 @@ This throttle is 'leading' and has some other properties that are specifically d Inspired by FluxML See: https://github.com/FluxML/Flux.jl/blob/8afedcd6723112ff611555e350a8c84f4e1ad686/src/utils.jl#L662 """ -function throttled(f::Function, timeout::Real; runtime_multiplier::Float64=0.0, run_threaded::Bool=false) +function throttled(f::Function, timeout::Real; runtime_multiplier::Float64=0.0) tlock = ReentrantLock() iscoolnow = Ref(false) run_later = Ref(false) last_runtime = Ref(0.0) - tf = ThrottledFunction(f, timeout, runtime_multiplier, tlock, iscoolnow, run_later, last_runtime, run_threaded) + tf = ThrottledFunction(f, timeout, runtime_multiplier, tlock, iscoolnow, run_later, last_runtime) # we initialize hot, and start the cooldown period immediately schedule(tf) From 06399a0167c96cab4f6809300526480f091e81d6 Mon Sep 17 00:00:00 2001 From: Fons van der Plas Date: Mon, 12 Aug 2024 15:12:22 +0200 Subject: [PATCH 05/11] cleanup --- src/evaluation/Run.jl | 2 +- src/webserver/Dynamic.jl | 23 +---------------------- 2 files changed, 2 insertions(+), 23 deletions(-) diff --git a/src/evaluation/Run.jl b/src/evaluation/Run.jl index 54c2a9591e..81be9f736b 100644 --- a/src/evaluation/Run.jl +++ b/src/evaluation/Run.jl @@ -123,7 +123,7 @@ function run_reactive_core!( # Send intermediate updates to the clients at most 20 times / second during a reactive run. (The effective speed of a slider is still unbounded, because the last update is not throttled.) # flush_send_notebook_changes_throttled, - send_notebook_changes_throttled = Throttled.throttled(1.0 / 20; runtime_multiplier=2.0, run_threaded=false) do + send_notebook_changes_throttled = Throttled.throttled(1.0 / 20; runtime_multiplier=2.0) do # We will do a state sync now, so that means that we can delay the status_tree state sync loop, see https://github.com/fonsp/Pluto.jl/issues/2978 Throttled.force_throttle_without_run(notebook.status_tree.update_listener_ref[]) # State sync: diff --git a/src/webserver/Dynamic.jl b/src/webserver/Dynamic.jl index 1feea48519..9a4ad699cc 100644 --- a/src/webserver/Dynamic.jl +++ b/src/webserver/Dynamic.jl @@ -166,16 +166,6 @@ const current_state_for_clients_lock = Base.Semaphore(1) dontacquire(f::Function, z) = f() -get_e_1(x) = try - join(x["status_tree"]["subtasks"]["run"]["subtasks"]["evaluate"]["subtasks"] |> keys, ", ") -catch - "empty" -end - -interesting_patches(patches) = filter(patch -> ["status_tree", "subtasks", "run", "subtasks", "evaluate", "subtasks"] ⊆ patch.path, patches) - - - const update_counter = Ref(0) @@ -197,12 +187,6 @@ function send_notebook_changes!(🙋::ClientRequest; commentary::Any=nothing, sk patches_as_dicts = Firebasey._convert(Vector{Dict}, patches) current_state_for_clients[client] = deep_enough_copy(notebook_dict) - let ip = interesting_patches(patches) - if !isempty(ip) - @debug "snc" get_e_1(current_dict) get_e_1(notebook_dict) ip counter - end - end - # Make sure we do send a confirmation to the client who made the request, even without changes is_response = 🙋.initiator !== nothing && client == 🙋.initiator.client @@ -210,8 +194,6 @@ function send_notebook_changes!(🙋::ClientRequest; commentary::Any=nothing, sk if !skip_send && (!isempty(patches) || is_response) response = Dict( :counter => counter, - :before_status => get_e_1(current_dict), - :after_status => get_e_1(notebook_dict), :patches => patches_as_dicts, :response => is_response ? commentary : nothing ) @@ -331,12 +313,9 @@ responses[:update_notebook] = function response_update_notebook(🙋::ClientRequ end # TODO Immutable ?? - # b = get_e_1(current_state_for_clients[🙋.initiator.client]) for patch in patches Firebasey.applypatch!(current_state_for_clients[🙋.initiator.client], patch) end - # a = get_e_1(current_state_for_clients[🙋.initiator.client]) - # @debug "un" b a changes = Set{Changed}() @@ -465,7 +444,7 @@ responses[:run_multiple_cells] = function response_run_multiple_cells(🙋::Clie # update current_state_for_clients for our client with c.queued = true. # later, during update_save_run!, the cell will actually run, eventually setting c.queued = false again, which will be sent to the client through a patch update. # This guarantees that something will be sent. - # We *need* to send *something* to the client, because of https://github.com/fonsp/Pluto.jl/pull/1892, but we also don't want to send unnecessary updates. We can skip sending this update with send_notebook_changes!, because update_save_run! will trigger a send_notebook_changes! very very soon. + # We *need* to send *something* to the client, because of https://g ithub.com/fonsp/Pluto.jl/pull/1892, but we also don't want to send unnecessary updates. We can skip sending this update with send_notebook_changes!, because update_save_run! will trigger a send_notebook_changes! very very soon. # send_notebook_changes!(🙋; skip_send=true) for (_, client) in 🙋.session.connected_clients From 48ed5ab45ee5ec1b38f89db70b01e06d665a25db Mon Sep 17 00:00:00 2001 From: Fons van der Plas Date: Mon, 12 Aug 2024 15:39:29 +0200 Subject: [PATCH 06/11] Update Dynamic.jl --- src/webserver/Dynamic.jl | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/src/webserver/Dynamic.jl b/src/webserver/Dynamic.jl index 9a4ad699cc..054be9a485 100644 --- a/src/webserver/Dynamic.jl +++ b/src/webserver/Dynamic.jl @@ -172,26 +172,24 @@ const update_counter = Ref(0) """ Update the local state of all clients connected to this notebook. """ -function send_notebook_changes!(🙋::ClientRequest; commentary::Any=nothing, skip_send::Bool=false, should_lock::Bool=true) +function send_notebook_changes!(🙋::ClientRequest; commentary::Any=nothing, should_lock::Bool=true) outbox = Set{Tuple{ClientSession,UpdateMessage}}() - (should_lock ? Base.acquire : dontacquire)(current_state_for_clients_lock) do + Base.acquire(current_state_for_clients_lock) do notebook_dict = notebook_to_js(🙋.notebook) + counter = update_counter[] += 1 + for (_, client) in 🙋.session.connected_clients if client.connected_notebook !== nothing && client.connected_notebook.notebook_id == 🙋.notebook.notebook_id current_dict = get(current_state_for_clients, client, :empty) - - counter = update_counter[] += 1 - patches = Firebasey.diff(current_dict, notebook_dict) patches_as_dicts = Firebasey._convert(Vector{Dict}, patches) current_state_for_clients[client] = deep_enough_copy(notebook_dict) - # Make sure we do send a confirmation to the client who made the request, even without changes is_response = 🙋.initiator !== nothing && client == 🙋.initiator.client - if !skip_send && (!isempty(patches) || is_response) + if !isempty(patches) || is_response response = Dict( :counter => counter, :patches => patches_as_dicts, @@ -299,7 +297,7 @@ const effects_of_changed_state = Dict( responses[:update_notebook] = function response_update_notebook(🙋::ClientRequest) require_notebook(🙋) try - Base.acquire(current_state_for_clients_lock) + # Base.acquire(current_state_for_clients_lock) notebook = 🙋.notebook patches = (Base.convert(Firebasey.JSONPatch, update) for update in 🙋.body["updates"]) @@ -376,7 +374,7 @@ responses[:update_notebook] = function response_update_notebook(🙋::ClientRequ ) send_notebook_changes!(🙋; commentary=response, should_lock=false) finally - Base.release(current_state_for_clients_lock) + # Base.release(current_state_for_clients_lock) end end @@ -445,7 +443,6 @@ responses[:run_multiple_cells] = function response_run_multiple_cells(🙋::Clie # later, during update_save_run!, the cell will actually run, eventually setting c.queued = false again, which will be sent to the client through a patch update. # This guarantees that something will be sent. # We *need* to send *something* to the client, because of https://g ithub.com/fonsp/Pluto.jl/pull/1892, but we also don't want to send unnecessary updates. We can skip sending this update with send_notebook_changes!, because update_save_run! will trigger a send_notebook_changes! very very soon. - # send_notebook_changes!(🙋; skip_send=true) for (_, client) in 🙋.session.connected_clients if client.connected_notebook !== nothing && client.connected_notebook.notebook_id == 🙋.notebook.notebook_id From c836a0c3323c02992481cec8a5cca238aeac8cf7 Mon Sep 17 00:00:00 2001 From: Fons van der Plas Date: Mon, 12 Aug 2024 17:19:47 +0200 Subject: [PATCH 07/11] cleanup --- frontend/components/Editor.js | 8 +--- frontend/imports/immer.js | 14 ------ src/evaluation/RunBonds.jl | 2 +- src/webserver/Dynamic.jl | 80 +++++++++++++++-------------------- 4 files changed, 37 insertions(+), 67 deletions(-) diff --git a/frontend/components/Editor.js b/frontend/components/Editor.js index 1081c761ce..7dbad96de7 100644 --- a/frontend/components/Editor.js +++ b/frontend/components/Editor.js @@ -699,10 +699,6 @@ export class Editor extends Component { // throw new Error(`Error: [Immer] minified error nr: 15 '${patches?.[0]?.path?.join("/")}' .`) // } - window.immer = immer - window.applyPatches = applyPatches - window.produceWithPatches = produceWithPatches - if (get_reverse_patches) { ;[new_notebook, _copy_of_patches, reverse_of_patches] = produceWithPatches(old_state ?? state.notebook, (state) => { applyPatches(state, patches) @@ -715,7 +711,6 @@ export class Editor extends Component { /** @type {String} Example: `"a.b[2].c"` */ const failing_path = String(exception).match(".*'(.*)'.*")?.[1].replace(/\//gi, ".") ?? exception const path_value = _.get(this.state.notebook, failing_path, "Not Found") - console.log(String(exception).match(".*'(.*)'.*")?.[1].replace(/\//gi, ".") ?? exception, failing_path, typeof failing_path) const ignore = should_ignore_patch_error(failing_path) @@ -738,8 +733,7 @@ all patches: ${JSON.stringify(patches, null, 1)} let parts = failing_path.split(".") for (let i = 0; i < parts.length; i++) { let path = parts.slice(0, i).join(".") - let value = _.get(this.state.notebook, path, "Not Found") - console.log(path, value) + console.log(path, _.get(this.state.notebook, path, "Not Found")) } if (ignore) { diff --git a/frontend/imports/immer.js b/frontend/imports/immer.js index 66af43e3cd..aec3b510ac 100644 --- a/frontend/imports/immer.js +++ b/frontend/imports/immer.js @@ -11,17 +11,3 @@ enablePatches() // The solution is to tell immer not to create immutable objects setAutoFreeze(false) - -// // @ts-nocheck -// import { produce as immer, produceWithPatches, applyPatches, enablePatches, setAutoFreeze } from "https://cdn.jsdelivr.net/npm/immer@10.1.1/dist/immer.mjs" - -// export { applyPatches, produceWithPatches } -// export default immer - -// enablePatches() - -// // we have some Editor.setState functions that use immer, so Editor.this.state becomes an "immer immutable frozen object". But we also have some Editor.setState functions that don't use immer, and they try to _mutate_ Editor.this.state. This gives errors like https://github.com/immerjs/immer/issues/576 - -// // The solution is to tell immer not to create immutable objects - -// setAutoFreeze(false) diff --git a/src/evaluation/RunBonds.jl b/src/evaluation/RunBonds.jl index a51ee0ac11..7ec4e191b6 100644 --- a/src/evaluation/RunBonds.jl +++ b/src/evaluation/RunBonds.jl @@ -33,7 +33,7 @@ function set_bond_values_reactive(; )::Vector{Symbol} if isempty(syms_to_set) || !will_run_code(notebook) - send_notebook_changes!(ClientRequest(; session, notebook, initiator); should_lock=false) + send_notebook_changes!(ClientRequest(; session, notebook, initiator)) return TopologicalOrder(notebook.topology, Cell[], Dict{Cell,PlutoDependencyExplorer.ReactivityError}()) end diff --git a/src/webserver/Dynamic.jl b/src/webserver/Dynamic.jl index 054be9a485..9826433683 100644 --- a/src/webserver/Dynamic.jl +++ b/src/webserver/Dynamic.jl @@ -162,22 +162,18 @@ end For each connected client, we keep a copy of their current state. This way we know exactly which updates to send when the server-side state changes. """ const current_state_for_clients = WeakKeyDict{ClientSession,Any}() -const current_state_for_clients_lock = Base.Semaphore(1) - -dontacquire(f::Function, z) = f() - -const update_counter = Ref(0) - +const current_state_for_clients_lock = ReentrantLock() +const update_counter_for_debugging = Ref(0) """ Update the local state of all clients connected to this notebook. """ -function send_notebook_changes!(🙋::ClientRequest; commentary::Any=nothing, should_lock::Bool=true) +function send_notebook_changes!(🙋::ClientRequest; commentary::Any=nothing) outbox = Set{Tuple{ClientSession,UpdateMessage}}() - Base.acquire(current_state_for_clients_lock) do + lock(current_state_for_clients_lock) do notebook_dict = notebook_to_js(🙋.notebook) - counter = update_counter[] += 1 + counter = update_counter_for_debugging[] += 1 for (_, client) in 🙋.session.connected_clients if client.connected_notebook !== nothing && client.connected_notebook.notebook_id == 🙋.notebook.notebook_id @@ -185,7 +181,7 @@ function send_notebook_changes!(🙋::ClientRequest; commentary::Any=nothing, sh patches = Firebasey.diff(current_dict, notebook_dict) patches_as_dicts = Firebasey._convert(Vector{Dict}, patches) current_state_for_clients[client] = deep_enough_copy(notebook_dict) - + # Make sure we do send a confirmation to the client who made the request, even without changes is_response = 🙋.initiator !== nothing && client == 🙋.initiator.client @@ -248,18 +244,6 @@ const effects_of_changed_state = Dict( @info "Process status set by client" newstatus end, - # "execution_allowed" => function(; request::ClientRequest, patch::Firebasey.ReplacePatch) - # Firebasey.applypatch!(request.notebook, patch) - # newstatus = patch.value - - # @info "execution_allowed set by client" newstatus - # if newstatus - # @info "lets run some cells!" - # update_save_run!(request.session, request.notebook, notebook.cells; - # run_async=true, save=true - # ) - # end - # end, "in_temp_dir" => function(; _...) no_changes end, "cell_inputs" => Dict( Wildcard() => function(cell_id, rest...; request::ClientRequest, patch::Firebasey.JSONPatch) @@ -297,12 +281,11 @@ const effects_of_changed_state = Dict( responses[:update_notebook] = function response_update_notebook(🙋::ClientRequest) require_notebook(🙋) try - # Base.acquire(current_state_for_clients_lock) notebook = 🙋.notebook patches = (Base.convert(Firebasey.JSONPatch, update) for update in 🙋.body["updates"]) if length(patches) == 0 - send_notebook_changes!(🙋; should_lock=false) + send_notebook_changes!(🙋) return nothing end @@ -364,7 +347,7 @@ responses[:update_notebook] = function response_update_notebook(🙋::ClientRequ ) end - send_notebook_changes!(🙋; commentary=Dict(:update_went_well => :👍), should_lock=false) + send_notebook_changes!(🙋; commentary=Dict(:update_went_well => :👍)) catch ex @error "Update notebook failed" 🙋.body["updates"] exception=(ex, stacktrace(catch_backtrace())) response = Dict( @@ -372,9 +355,7 @@ responses[:update_notebook] = function response_update_notebook(🙋::ClientRequ :why_not => sprint(showerror, ex), :should_i_tell_the_user => ex isa SessionActions.UserError, ) - send_notebook_changes!(🙋; commentary=response, should_lock=false) - finally - # Base.release(current_state_for_clients_lock) + send_notebook_changes!(🙋; commentary=response) end end @@ -430,6 +411,29 @@ responses[:reset_shared_state] = function response_reset_shared_state(🙋::Clie end end +""" +This is a little hack to solve https://github.com/fonsp/Pluto.jl/pull/1892 + +This function updates current_state_for_clients for our client with cell.queued = true. + +Later, during update_save_run!, the cell will actually run, eventually setting cell.queued = false again, which will be sent to the client through a patch update. +This guarantees that something will be sent. + +We *need* to send *something* to the client, because of https://github.com/fonsp/Pluto.jl/pull/1892, but we also don't want to send unnecessary updates. We can do this instead of a regular call to `send_notebook_changes!`, because update_save_run! will trigger a send_notebook_changes! call very very soon. +""" +function _set_cells_to_queued_in_local_state(client, notebook, cells) + if haskey(current_state_for_clients, client) + results = current_state_for_clients[client]["cell_results"] + for cell in cells + if haskey(results, cell.cell_id) + old = results[cell.cell_id]["queued"] + results[cell.cell_id]["queued"] = true + @debug "Setting val!" cell.cell_id old + end + end + end +end + responses[:run_multiple_cells] = function response_run_multiple_cells(🙋::ClientRequest) require_notebook(🙋) uuids = UUID.(🙋.body["cells"]) @@ -438,23 +442,9 @@ responses[:run_multiple_cells] = function response_run_multiple_cells(🙋::Clie end if will_run_code(🙋.notebook) - foreach(c -> c.queued = true, cells) - # update current_state_for_clients for our client with c.queued = true. - # later, during update_save_run!, the cell will actually run, eventually setting c.queued = false again, which will be sent to the client through a patch update. - # This guarantees that something will be sent. - # We *need* to send *something* to the client, because of https://g ithub.com/fonsp/Pluto.jl/pull/1892, but we also don't want to send unnecessary updates. We can skip sending this update with send_notebook_changes!, because update_save_run! will trigger a send_notebook_changes! very very soon. - - for (_, client) in 🙋.session.connected_clients - if client.connected_notebook !== nothing && client.connected_notebook.notebook_id == 🙋.notebook.notebook_id - if haskey(current_state_for_clients, client) - results = current_state_for_clients[client]["cell_results"] - for c in cells - if haskey(results, cell.cell_id) - results[cell.cell_id]["queued"] = true - end - end - end - end + foreach(cell -> cell.queued = true, cells) + if 🙋.initiator.client !== nothing + _set_cells_to_queued_in_local_state(🙋.initiator.client, 🙋.notebook, cells) end end From bc4a40c25f69d613fdbe63376c956413c1889c28 Mon Sep 17 00:00:00 2001 From: Fons van der Plas Date: Tue, 13 Aug 2024 00:21:39 +0200 Subject: [PATCH 08/11] Update Dynamic.jl --- src/webserver/Dynamic.jl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/webserver/Dynamic.jl b/src/webserver/Dynamic.jl index 9826433683..3e4baa4f7c 100644 --- a/src/webserver/Dynamic.jl +++ b/src/webserver/Dynamic.jl @@ -443,7 +443,7 @@ responses[:run_multiple_cells] = function response_run_multiple_cells(🙋::Clie if will_run_code(🙋.notebook) foreach(cell -> cell.queued = true, cells) - if 🙋.initiator.client !== nothing + if 🙋.initiator !== nothing _set_cells_to_queued_in_local_state(🙋.initiator.client, 🙋.notebook, cells) end end From d8d0ba4b72a5c7b316831f52a7320adb7bbae317 Mon Sep 17 00:00:00 2001 From: Fons van der Plas Date: Tue, 13 Aug 2024 12:51:34 +0200 Subject: [PATCH 09/11] move outbox --- src/webserver/Dynamic.jl | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/webserver/Dynamic.jl b/src/webserver/Dynamic.jl index 3e4baa4f7c..1fc38c1d5f 100644 --- a/src/webserver/Dynamic.jl +++ b/src/webserver/Dynamic.jl @@ -195,12 +195,12 @@ function send_notebook_changes!(🙋::ClientRequest; commentary::Any=nothing) end end end + end - for (client, msg) in outbox - putclientupdates!(client, msg) - end - try_event_call(🙋.session, FileEditEvent(🙋.notebook)) + for (client, msg) in outbox + putclientupdates!(client, msg) end + try_event_call(🙋.session, FileEditEvent(🙋.notebook)) end "Like `deepcopy`, but anything other than `Dict` gets a shallow (reference) copy." From 8a50aaacca8738ffbf9306f03e4a421b4e48111d Mon Sep 17 00:00:00 2001 From: Fons van der Plas Date: Wed, 14 Aug 2024 22:15:28 +0200 Subject: [PATCH 10/11] =?UTF-8?q?Remove=20the=20`cell.queued`=20"dirty=20h?= =?UTF-8?q?ack"=20=F0=9F=98=BA?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- frontend/components/Editor.js | 14 +------------- src/webserver/Dynamic.jl | 26 -------------------------- 2 files changed, 1 insertion(+), 39 deletions(-) diff --git a/frontend/components/Editor.js b/frontend/components/Editor.js index b6cc050877..b4871bcdff 100644 --- a/frontend/components/Editor.js +++ b/frontend/components/Editor.js @@ -614,19 +614,7 @@ export class Editor extends Component { } } }) - // This is a "dirty" trick, as this should actually be stored in some shared request_status => status state - // But for now... this is fine 😼 - await this.setStatePromise( - immer((/** @type {EditorState} */ state) => { - for (let cell_id of cell_ids) { - if (state.notebook.cell_results[cell_id] != null) { - state.notebook.cell_results[cell_id].queued = this.is_process_ready() - } else { - // nothing - } - } - }) - ) + const result = await this.client.send("run_multiple_cells", { cells: cell_ids }, { notebook_id: this.state.notebook.notebook_id }) const { disabled_cells } = result.message if (Object.entries(disabled_cells).length > 0) { diff --git a/src/webserver/Dynamic.jl b/src/webserver/Dynamic.jl index 1fc38c1d5f..f79d9d5f9f 100644 --- a/src/webserver/Dynamic.jl +++ b/src/webserver/Dynamic.jl @@ -411,29 +411,6 @@ responses[:reset_shared_state] = function response_reset_shared_state(🙋::Clie end end -""" -This is a little hack to solve https://github.com/fonsp/Pluto.jl/pull/1892 - -This function updates current_state_for_clients for our client with cell.queued = true. - -Later, during update_save_run!, the cell will actually run, eventually setting cell.queued = false again, which will be sent to the client through a patch update. -This guarantees that something will be sent. - -We *need* to send *something* to the client, because of https://github.com/fonsp/Pluto.jl/pull/1892, but we also don't want to send unnecessary updates. We can do this instead of a regular call to `send_notebook_changes!`, because update_save_run! will trigger a send_notebook_changes! call very very soon. -""" -function _set_cells_to_queued_in_local_state(client, notebook, cells) - if haskey(current_state_for_clients, client) - results = current_state_for_clients[client]["cell_results"] - for cell in cells - if haskey(results, cell.cell_id) - old = results[cell.cell_id]["queued"] - results[cell.cell_id]["queued"] = true - @debug "Setting val!" cell.cell_id old - end - end - end -end - responses[:run_multiple_cells] = function response_run_multiple_cells(🙋::ClientRequest) require_notebook(🙋) uuids = UUID.(🙋.body["cells"]) @@ -443,9 +420,6 @@ responses[:run_multiple_cells] = function response_run_multiple_cells(🙋::Clie if will_run_code(🙋.notebook) foreach(cell -> cell.queued = true, cells) - if 🙋.initiator !== nothing - _set_cells_to_queued_in_local_state(🙋.initiator.client, 🙋.notebook, cells) - end end function on_auto_solve_multiple_defs(disabled_cells_dict) From da6d31c4b824919c50678cc2fc6647aa67778bfa Mon Sep 17 00:00:00 2001 From: Fons van der Plas Date: Wed, 14 Aug 2024 22:20:42 +0200 Subject: [PATCH 11/11] update comment --- frontend/components/CellOutput.js | 2 +- src/webserver/Dynamic.jl | 9 ++------- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/frontend/components/CellOutput.js b/frontend/components/CellOutput.js index d274fea663..1aa16bff8b 100644 --- a/frontend/components/CellOutput.js +++ b/frontend/components/CellOutput.js @@ -479,7 +479,7 @@ const execute_scripttags = async ({ root_node, script_nodes, previous_results_ma let run = (f) => f() /** - * Support declarative shadowroot 😼 + * Support declarative shadowroot 😺 * https://web.dev/declarative-shadow-dom/ * The polyfill they mention on the page is nice and all, but we need more. * For one, we need the polyfill anyway as we're adding html using innerHTML (just like we need to run the scripts ourselves) diff --git a/src/webserver/Dynamic.jl b/src/webserver/Dynamic.jl index 1fc38c1d5f..59a3aaad64 100644 --- a/src/webserver/Dynamic.jl +++ b/src/webserver/Dynamic.jl @@ -412,14 +412,9 @@ responses[:reset_shared_state] = function response_reset_shared_state(🙋::Clie end """ -This is a little hack to solve https://github.com/fonsp/Pluto.jl/pull/1892 +This function updates current_state_for_clients for our client with `cell.queued = true`. We do the same on the client side, where we set the cell's result to `queued = true` immediately in that client's local state. Search for `😼` in the frontend code. -This function updates current_state_for_clients for our client with cell.queued = true. - -Later, during update_save_run!, the cell will actually run, eventually setting cell.queued = false again, which will be sent to the client through a patch update. -This guarantees that something will be sent. - -We *need* to send *something* to the client, because of https://github.com/fonsp/Pluto.jl/pull/1892, but we also don't want to send unnecessary updates. We can do this instead of a regular call to `send_notebook_changes!`, because update_save_run! will trigger a send_notebook_changes! call very very soon. +This is also kinda related to https://github.com/fonsp/Pluto.jl/pull/1892 but not really, see https://github.com/fonsp/Pluto.jl/pull/2989. I actually think this does not make a differency anymore, see https://github.com/fonsp/Pluto.jl/pull/2999. """ function _set_cells_to_queued_in_local_state(client, notebook, cells) if haskey(current_state_for_clients, client)