Conversation
…ples Follow-up to the interruptible-teardown work, addressing review feedback. Shutdown flags are now published under the mutex that the condition variable predicates read them under. Setting an atomic outside that mutex and then notifying leaves a window in which a waiter that has just evaluated its predicate as false enters the wait and misses the notification, so the offset thread could still sleep out its full five-second interval. The blocking calls that a stop could not interrupt are now issued in short slices that observe the shutdown flag: - stream_inlet::info() was called twice with the default infinite timeout. close_stream() only stops the data receiver, so an unreachable metadata endpoint blocked a stop indefinitely. The info is now fetched once and reused for the header and the nominal rate. - open_stream() could hold a stop for up to max_open_wait. - time_correction() could hold it for the full query timeout. - The watchlist resolver blocked for a whole resolve_interval; it now resolves briefly and waits out the rest interruptibly. - The phase gates could park a stream for max_headers_wait with no way out, so a stream could lose its footer waiting for one that had hung. Joining is bounded for the first time: try_join_once() called std::thread::join(), which has no timeout, so polling it could never enforce max_join_wait. Threads are now paired with a future that becomes ready when the body returns, which can be waited on with a deadline. Closing the inlets the moment stop is pressed discards everything still buffered in them; a recording of 40 markers came back with 2. Inlets are now closed only after the stream threads have been given a grace period to drain and write their footers, and the transfer loop does a final non-blocking pull on the way out, so a stop no longer costs samples that had already arrived. Also fixed along the way: record_offsets() wrote uninitialised offset and timestamp values into the file when a time correction query timed out; the inlet bookkeeping leaked a registration on every exception path; and a stream that failed mid-recording was left without a footer although its header was already on disk. scripts/test_recording_teardown.py now covers a plain stop, a stop before the first sample, a stop while subscribing to a source that has gone away, repeated start/stop cycles, and that no buffered sample is lost. It checks the exit status and the footers of every stream, holds one stated teardown budget instead of documenting one and asserting another, and runs on all three platforms in CI.
…in a gated stream Addresses the second round of review on the teardown work. The future-based join added in the previous commit is the first version that can actually reach the detach path, because the blocking join() it replaced never returned for a hung thread. Detached threads ran on a raw `this` and wrote into the file, the mutexes and the offset lists, all of which the destructor had already destroyed. Recording state and the thread bodies now live in a shared implementation object that every thread holds a reference to, so the state outlives a teardown that had to leave a thread running, and the last one to finish closes the file. A harness that stalls a stream thread past the join deadline faults with an access violation and writes no footer before this change, and exits cleanly with both footers intact after it. The gate that lets a stream wait for another stream's header is released on shutdown, which meant a stream could reach its transfer loop with the shutdown already set, pull no first sample, and then skip the final drain because it had no first timestamp to compare against -- writing a zero-sample footer although its inlet had been subscribed and buffering the whole time. The first sample now anchors the stream wherever it arrives from, including from the drain, and the drain is unconditional. Two things found while testing this: Splitting the time correction query into network_poll_interval slices was unsound: the query needs a round trip to complete, so restarting it every 200 ms means it need never finish. It goes back to a single call with the full budget, as before the teardown work. Teardown stays bounded because the transfer thread now stops waiting for the offset thread after the teardown grace period and leaves it running, which is safe now that a thread left running keeps its state alive. Recording threads all logged through unsynchronised << chains, so their output interleaved mid-line. Lines are now composed and written under a mutex. scripts/test_recording_teardown.py gains a case for the gated stream: it holds one stream at the headers gate behind another whose source has gone away, buffers 40 samples into it and stops. Before the drain fix that case records 0 of 40.
Poll clock correction without restarting liblsl's background measurement, and join every worker before releasing the writer. A slow worker must not leave the CLI with buffered output owned by a detached thread at process exit. Add deterministic finalization regressions to CI and verify real clock offsets in the integration suite.
…ed exit Keep the interface responsive while workers drain and the writer closes. Report finishing and stalled states, require confirmation for GUI force quit, and enforce a configurable CLI finalization deadline with nonzero exit on timeout. Checkpoint complete XDF chunks, flush footers immediately, and report worker or output errors instead of premature success. Preserve interruptible waits and buffered-sample draining. Add backend, CLI, and Qt state-machine regressions including a permanently stalled worker and forced process exit.
|
Thanks -- @cboulay, I think this is an improvement, I iterated on a review with a Claude Opus 5 Max agent, please see the points below, especially 1-3. Reviewed by source inspection against 1. Sustained throughput is capped at ~2 kHz per stream
Any stream above ~2 kHz now falls permanently behind, the inlet backlog grows until liblsl's 360 s buffer is full, and samples are dropped at the source. 1 kHz-5 kHz EEG is common with this tool. Nothing in the suite detects it: Minimal fix: skip the 2.
|
2 participants
Builds on Stefan Appelhoff’s (@sappelhoff) work in #146, retaining its commits and integration tests. This draft proposes a follow-up design for the same GUI-freeze and incomplete-XDF-footer problems.
Stop now captures a recorder-clock cutoff and starts asynchronous finalization. Inlets stay subscribed while delayed pre-stop timestamps advance, so a stream that is catching up can continue beyond five seconds. Clock correction is used for cutoff comparisons without rewriting stored timestamps or correcting an already synchronized inlet twice. Bounded receive batches keep stop checks responsive.
The GUI distinguishes catching up, waiting, and closing the file. Finish now ends collection early while preserving normal footer/close handling; an explicit force-quit option remains available for stalled workers or file operations. The CLI timeout measures inactivity, and progress on one inlet cannot conceal another inlet’s stall. Completion is reported only after workers finish and the writer is flushed and closed.
Each stream has a two-second grace without advancing pre-stop timestamps, including a window for reordered samples. The footer records the cutoff and why collection ended; the GUI notes streams that ended without observing the cutoff. This is a bounded fallback for silent streams and unreliable clock/timestamp information, not a guarantee that upstream buffers are empty. Data arriving after the grace can be missed, and force quitting can still leave an incomplete file.
Validation on macOS with liblsl v1.18.0.b3:
Related: #146