From bd67344f8f4f4f1850a4c1829bf03917e09433d1 Mon Sep 17 00:00:00 2001 From: Fons van der Plas Date: Thu, 15 Aug 2024 09:56:18 +0200 Subject: [PATCH] Fix state out of sync issue caused by race condition (#2989) --- frontend/components/CellOutput.js | 2 +- frontend/components/Editor.js | 7 +++++ src/webserver/Dynamic.jl | 51 ++++++++++++++++++------------- 3 files changed, 38 insertions(+), 22 deletions(-) diff --git a/frontend/components/CellOutput.js b/frontend/components/CellOutput.js index f438ab2337..cf3d418d2d 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/frontend/components/Editor.js b/frontend/components/Editor.js index f1a874958b..c063a1712f 100644 --- a/frontend/components/Editor.js +++ b/frontend/components/Editor.js @@ -729,10 +729,17 @@ 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(".") + console.log(path, _.get(this.state.notebook, path, "Not Found")) + } + if (ignore) { console.info("Safe to ignore this patch failure...") } else if (this.state.connected) { diff --git a/src/webserver/Dynamic.jl b/src/webserver/Dynamic.jl index 76548c427d..59a3aaad64 100644 --- a/src/webserver/Dynamic.jl +++ b/src/webserver/Dynamic.jl @@ -163,15 +163,18 @@ For each connected client, we keep a copy of their current state. This way we kn """ const current_state_for_clients = WeakKeyDict{ClientSession,Any}() 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, skip_send::Bool=false) +function send_notebook_changes!(🙋::ClientRequest; commentary::Any=nothing) outbox = Set{Tuple{ClientSession,UpdateMessage}}() lock(current_state_for_clients_lock) do notebook_dict = notebook_to_js(🙋.notebook) + 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 current_dict = get(current_state_for_clients, client, :empty) @@ -182,8 +185,9 @@ function send_notebook_changes!(🙋::ClientRequest; commentary::Any=nothing, sk # 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, :response => is_response ? commentary : nothing ) @@ -192,7 +196,7 @@ function send_notebook_changes!(🙋::ClientRequest; commentary::Any=nothing, sk end end end - + for (client, msg) in outbox putclientupdates!(client, msg) end @@ -240,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) @@ -339,7 +331,7 @@ 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 let bond_changes = filter(x -> x isa BondChanged, changes) @@ -419,6 +411,24 @@ responses[:reset_shared_state] = function response_reset_shared_state(🙋::Clie end end +""" +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 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) + 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"]) @@ -427,11 +437,10 @@ responses[:run_multiple_cells] = function response_run_multiple_cells(🙋::Clie end 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. - # 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) + 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)