Skip to content

Drain held events once connected - #8698

Merged
philippjfr merged 5 commits into
mainfrom
hold_drain
Aug 4, 2026
Merged

Drain held events once connected#8698
philippjfr merged 5 commits into
mainfrom
hold_drain

Conversation

@philippjfr

@philippjfr philippjfr commented Aug 4, 2026

Copy link
Copy Markdown
Member

Fixes #8692.

Background

When a hold() block exits while the Document is not yet connected, the finally chain in panel/io/document.py took:

elif not state._connected.get(doc):
    doc.callbacks._hold = None

This clears the hold policy but leaves doc.callbacks._held_events populated. Bokeh's unhold() returns early when _hold is None, so nothing drains them there. The events sit on the Document until some later, unrelated hold()/unhold() pair flushes them.

The branch is deliberate. It arrived in #7972 to avoid dispatching events before the Document is fully initialized, which leads to the pending writes error. The intent was that these events be discarded rather than deferred: model changes occurring during startup don't need to be synced, because they are picked up anyway when the whole Document is serialized on connect.

The problem

That premise only holds if the change actually reaches the model, and pre-connect it often doesn't.

state._unblocked(doc) is False before the session connects, so Reactive._apply_update takes its else branch and schedules the update via state.execute(cb, schedule=True), which is doc.add_next_tick_callback. That emits a SessionCallbackAdded, which is not a DocumentPatchedEvent and so is neither dispatched nor recoverable from a full serialization. Stranding it means the callback is never registered on the IOLoop and the deferred _update_model never runs.

So the single SessionCallbackAdded in the issue's reproducer is the pane update. Instrumenting that reproducer to also read the model:

on exiting hold: {'connected': None, 'hold': None, 'queued': ['SessionCallbackAdded', 'SessionCallbackAdded']}
model text at end:  ['<p>initial</p>\n', None, None]     # never became "changed"
inner next-tick callbacks that ran: []
doc pending session callbacks: 2

This is not late or out-of-context dispatch. The server-side model is left stale, and the full serialization on connect then faithfully sends the stale state. The same applies to anything else that registers through SessionCallbackAdded: add_next_tick_callback, add_periodic_callback, state.execute(schedule=True) and the scheduled branch of Reactive._send_event.

Two further gaps in the same chain:

The threaded branch never consulted _connected at all. It unconditionally scheduled doc.unhold() on a next tick, so the discard policy was simply not implemented there, and the dispatch it did instead is the pending-writes hazard #7972 was avoiding, reached by a different door. With config.nthreads set this is a common path, since _schedule_on_load submits _on_load to the thread pool.

The non-threaded branch mutated _hold without taking _HOLD_LOCK[doc], so it raced the threaded unhold.

The fix

Resolve the events at hold exit rather than parking them, sorting into three groups instead of discarding wholesale:

  • Dispatched immediatelySessionCallbackAdded / SessionCallbackRemoved. These are how callbacks get registered on the IOLoop; dropping them loses the work they represent. Dispatching pre-connect is safe because the Document lock is held at this point.
  • Deferred until connectMessageSentEvent. It is a DocumentPatchedEvent, but it carries protocol messages (custom events from Reactive._send_event, ipywidgets comm messages) with no representation in the model graph, so a full serialization cannot reproduce them. They cannot be dispatched at hold exit either, because there are no subscribed connections yet to write them to, so they are held in a _UNCONNECTED_EVENTS weak-keyed map and flushed from _on_load once _connected is set. The flush goes through a next-tick callback so it lands on the Document's thread with the lock held, since _on_load may run on the thread pool.
  • Dropped — everything else. ModelChangedEvent, TitleChangedEvent, RootAdded/RootRemovedEvent and the ColumnDataSource stream/patch hints are all applied to the model before the event is emitted, so the serialization on connect reproduces them. Worth noting for the CDS case: PropertyValueColumnData._stream and ._patch deliberately bypass the wrapped dict methods and write through to the underlying data, so .data is locally correct and the events are transport hints rather than the state itself.

The threaded branch now applies the same policy, so both agree on what happens pre-connect, and the drain takes _HOLD_LOCK[doc]. Events are collected under the lock but dispatched after releasing it, since the lock is not reentrant and a change callback may itself enter hold().

_UNCONNECTED_EVENTS is cleared in _destroy_document alongside the other per-Document maps.

Testing

Four tests in panel/tests/io/test_document.py:

  • test_hold_before_connected_does_not_strand_events — nothing is left queued, and the scheduled callback runs so the model update lands.
  • test_hold_before_connected_defers_message_sentMessageSentEvent is neither dropped nor stranded.
  • test_hold_before_connected_drops_recoverable_events — a model change is dropped, with the value present on the model.
  • test_threaded_hold_before_connected_does_not_strand_events — the threaded branch leaves no hold in place while unconnected. Asserts it actually took the threaded path.

The first and last were confirmed to fail against the pre-fix code (queued == ['SessionCallbackAdded'] and hold_value == 'combine' respectively), so they pin the regression rather than just passing.

panel/tests/io/, panel/tests/test_server.py and panel/tests/test_reactive.py show no regressions. Three test_server_ico_handling* failures and the test_resources/test_reactive stylesheet failures are present on main in this working tree and unrelated to this change.

🤖 Generated with Claude Code

@codecov

codecov Bot commented Aug 4, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.00000% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.14%. Comparing base (f1c0bb4) to head (d884c27).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
panel/io/document.py 85.71% 5 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main    #8698    +/-   ##
========================================
  Coverage   86.13%   86.14%            
========================================
  Files         348      348            
  Lines       57171    57287   +116     
========================================
+ Hits        49247    49350   +103     
- Misses       7924     7937    +13     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread panel/io/document.py Outdated
@philippjfr
philippjfr merged commit dc78597 into main Aug 4, 2026
1 of 2 checks passed
@philippjfr
philippjfr deleted the hold_drain branch August 4, 2026 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

hold() leaves events queued on the Document when the session is not connected

1 participant