feat: report/propagate client start/run failures#432
Draft
xdustinface wants to merge 2 commits intov0.42-devfrom
Draft
feat: report/propagate client start/run failures#432xdustinface wants to merge 2 commits intov0.42-devfrom
xdustinface wants to merge 2 commits intov0.42-devfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a fatal client error callback API (set/clear) with C header and docs updates, implements types and dispatch in Rust, wires client and sync threads to invoke the callback on fatal errors, refactors sync coordinators to propagate errors, and adds unit tests for callback behavior. Changes
Sequence DiagramsequenceDiagram
actor App as Application
participant C as C API
participant Client as FFIDashSpvClient
participant Monitor as Sync Monitor Thread
participant Callback as FFIClientErrorCallback
App->>C: dash_spv_ffi_client_set_client_error_callback(cb, user_data)
C->>Client: store callback (Arc<Mutex<Option<FFIClientErrorCallback>>>)
C-->>App: return status
App->>C: dash_spv_ffi_client_run()
C->>Client: spawn Monitor thread
Monitor->>Monitor: perform start / sync operations
alt fatal error occurs
Monitor->>Client: lock & read client_error_callback
Client-->>Monitor: callback ref (if set)
Monitor->>Callback: dispatch(error_str)
Callback->>App: on_error(error_ptr, user_data)
end
Monitor->>Client: continue or shutdown
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@dash-spv/src/client/sync_coordinator.rs`:
- Around line 61-63: The select arm currently ignores the Result from
progress_updates.changed(), causing a busy-loop if the watch sender is dropped;
change the match on progress_updates.changed() instead of using `_ =`, and on
Ok(_) log the progress with progress_updates.borrow() and return/continue as
intended, but on Err(RecvError) break the loop (e.g., `break None`) or otherwise
disable the arm so it doesn't repeatedly resolve immediately; update the select
arm handling of the `progress_updates` watch in the sync coordinator so the
closed-channel case is explicitly handled.
🧹 Nitpick comments (2)
dash-spv-ffi/tests/unit/test_client_lifecycle.rs (1)
213-277: Timing-dependent assertion may be flaky on slow CI.Line 255 uses a fixed
500mssleep to wait for the sync thread to fail and invoke the callback. On heavily loaded CI runners this could be insufficient, leading to intermittent failures.Consider retrying the assertion in a polling loop with a timeout instead of a single sleep.
🔧 Suggested polling approach
- // Give the sync thread time to start and fail - thread::sleep(Duration::from_millis(500)); - - let received_error = error_store.lock().unwrap(); - assert!( - received_error.is_some(), - "Client error callback should have been called on start failure" - ); + // Poll for the callback to fire, with a generous timeout + let deadline = std::time::Instant::now() + Duration::from_secs(5); + loop { + if error_store.lock().unwrap().is_some() { + break; + } + assert!( + std::time::Instant::now() < deadline, + "Client error callback should have been called on start failure" + ); + thread::sleep(Duration::from_millis(50)); + } + + let received_error = error_store.lock().unwrap();dash-spv/src/client/sync_coordinator.rs (1)
54-58: Consider whether everyhandle_commanderror should be fatal to the monitoring loop.Currently, if
handle_commandreturns an error (e.g.,ChannelFailurebecause a requester dropped its oneshot receiver before the response was sent), the entiremonitor_networkloop terminates. A caller dropping its response channel is arguably not a client-fatal condition — it's a transient per-request failure.You could log-and-continue for non-fatal command errors to avoid tearing down the whole client:
Possible approach
- Some(command) => self.handle_command(command).await.err(), + Some(command) => { + if let Err(e) = self.handle_command(command).await { + tracing::warn!("Command handling error: {}", e); + } + None + },If the intent is truly "any command failure = shut down", that's fine — just wanted to flag it.
xdustinface
force-pushed
the
feat/monitor-network-errors
branch
from
19354bc to
532083b
Compare
github-actions
bot
added
the
merge-conflict
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
xdustinface
force-pushed
the
feat/monitor-network-errors
branch
from
532083b to
97f4788
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
dash-spv-ffi/src/callbacks.rs (2)
591-592: MissingSAFETYcomments onunsafe impl Send/unsafe impl Sync.
FFISyncEventCallbacksdocuments why itsSend + Syncimpls are sound (function pointers are inherently thread-safe;user_datathread-safety is the caller's responsibility).FFIClientErrorCallbackshould carry the same explanation for consistency and auditability.✏️ Proposed addition
+// SAFETY: FFIClientErrorCallback is safe to send/share between threads because: +// 1. The on_error function pointer is an extern "C" fn with no captured state. +// 2. Thread safety of user_data is the caller's responsibility. unsafe impl Send for FFIClientErrorCallback {} unsafe impl Sync for FFIClientErrorCallback {}Based on learnings: "Exercise careful memory safety handling at FFI boundaries."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv-ffi/src/callbacks.rs` around lines 591 - 592, Add a SAFETY comment above the two unsafe impls for FFIClientErrorCallback explaining why implementing Send and Sync is sound: state that the struct only contains raw function pointers (which are inherently thread-safe) and a user_data pointer whose thread-safety is the caller's responsibility, mirroring the justification used for FFISyncEventCallbacks; ensure the comment mentions any invariants the caller must uphold (e.g., user_data must remain valid for the callback's lifetime and be accessed safely across threads).
572-611:FFIClientErrorCallbackis placed betweenFFIWalletEventCallbacks's struct definition and itsdispatchimpl, fragmenting the wallet type.The
FFIWalletEventCallbacksstruct is declared at line 551, but itsdispatchimpl doesn't appear until line 613 because lines 572–611 insert the entirely unrelatedFFIClientErrorCallbacktype. MovingFFIClientErrorCallbackto after theFFIWalletEventCallbacks::dispatchimpl (after line 664) would restore cohesion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv-ffi/src/callbacks.rs` around lines 572 - 611, FFIClientErrorCallback is interrupting the logical grouping of FFIWalletEventCallbacks by being declared between the FFIWalletEventCallbacks struct and its impl dispatch; move the entire FFIClientErrorCallback declaration (type alias, struct, unsafe impls, Default impl, and impl dispatch) to below the FFIWalletEventCallbacks::dispatch impl so the wallet struct and its methods remain contiguous, keeping the symbols FFIWalletEventCallbacks and its dispatch impl together and relocating FFIClientErrorCallback (including its OnClientErrorCallback, Default, unsafe impls, and impl dispatch) after that block.dash-spv-ffi/src/client.rs (2)
406-410: Redundant explicitdrop(guard).The
guardgoes out of scope at the end of theif let Err(e)block regardless. The explicitdropon line 410 is unnecessary and is inconsistent with the analogous start-failure path (lines 397–401) which relies on implicit drop.✂️ Proposed cleanup
let guard = error_callback.lock().unwrap(); if let Some(cb) = guard.as_ref() { cb.dispatch(&e.to_string()); } - drop(guard);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv-ffi/src/client.rs` around lines 406 - 410, The explicit drop(guard) after dispatching the error callback is redundant because guard (from let guard = error_callback.lock().unwrap()) will be dropped at the end of the if let Err(e) block; remove the drop(guard) call to match the implicit-drop style used in the start-failure path and avoid unnecessary code. Ensure you update the block around error_callback.lock().unwrap(), cb.dispatch(&e.to_string()), and the if let Err(e) { ... } so no explicit drop is present.
397-410: Document the re-entrancy / deadlock constraint in the safety docs forset_client_error_callback.Both dispatch sites (start failure at lines 397–401 and monitor error at lines 406–410) invoke
cb.dispatch()while holdingerror_callback.lock(). If the Con_errorimplementation calls back intodash_spv_ffi_client_set_client_error_callbackordash_spv_ffi_client_clear_client_error_callbackon the same thread, both functions will attempt to acquire the samestd::sync::Mutex, deadlocking.The
# Safetydoc fordash_spv_ffi_client_set_client_error_callbackshould explicitly prohibit re-entrant calls into the callback management API:📝 Proposed doc addition (lines 829–833)
/// # Safety /// - `client` must be a valid, non-null pointer to an `FFIDashSpvClient`. /// - The `callback` struct and its `user_data` must remain valid until the callback is cleared. /// - The callback must be thread-safe as it may be called from a background thread. +/// - The callback **must not** call `dash_spv_ffi_client_set_client_error_callback` or +/// `dash_spv_ffi_client_clear_client_error_callback` from within the callback body; +/// doing so will deadlock on the internal mutex.Based on learnings: "Exercise careful memory safety handling at FFI boundaries."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv-ffi/src/client.rs` around lines 397 - 410, The callback dispatch calls (cb.dispatch) occur while holding the error_callback Mutex in both the startup error path and in monitor_network, which can deadlock if the C on_error handler calls back into dash_spv_ffi_client_set_client_error_callback or dash_spv_ffi_client_clear_client_error_callback; update the safety docs for dash_spv_ffi_client_set_client_error_callback (and mention dash_spv_ffi_client_clear_client_error_callback) to explicitly prohibit re-entrant calls into the callback-management API and note the Mutex/deadlock risk, and preferably change the dispatch sites to drop the guard (release error_callback.lock()) before calling cb.dispatch or document that callers must not call those FFI functions from the callback (choose one approach and state it clearly in the # Safety text).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dash-spv-ffi/tests/unit/test_client_lifecycle.rs`:
- Around line 218-267: The test frees the Sender early
(drop(Box::from_raw(tx_ptr))) while the client's sync thread (spawned by
dash_spv_ffi_client_run and running monitor_network()) may still call the FFI
callback on_error via the dangling user_data pointer; move the
drop(Box::from_raw(tx_ptr)) so it happens only after calling
dash_spv_ffi_client_stop (and dash_spv_ffi_client_destroy if needed) to ensure
threads have been cancelled/joined, and optionally replace the thread::sleep
readiness heuristic with a synchronization primitive or channel from the sync
thread to the test to reliably know when monitor_network() is running before
attempting the second run.
In `@dash-spv/src/sync/sync_coordinator.rs`:
- Around line 237-244: Add a new SyncError::TaskPanicked variant in error.rs
(include a String or JoinError-wrapping payload and Display/From impls as
needed), then update the panic-handling branch in sync_coordinator.rs (the
Err(e) arm that currently returns SyncError::InvalidState(...)) to return
SyncError::TaskPanicked(format!("Manager task panicked: {}", e)) instead of
using InvalidState; ensure any call sites or conversions compile by adding
From<JoinError> for SyncError or matching construction in place.
---
Nitpick comments:
In `@dash-spv-ffi/src/callbacks.rs`:
- Around line 591-592: Add a SAFETY comment above the two unsafe impls for
FFIClientErrorCallback explaining why implementing Send and Sync is sound: state
that the struct only contains raw function pointers (which are inherently
thread-safe) and a user_data pointer whose thread-safety is the caller's
responsibility, mirroring the justification used for FFISyncEventCallbacks;
ensure the comment mentions any invariants the caller must uphold (e.g.,
user_data must remain valid for the callback's lifetime and be accessed safely
across threads).
- Around line 572-611: FFIClientErrorCallback is interrupting the logical
grouping of FFIWalletEventCallbacks by being declared between the
FFIWalletEventCallbacks struct and its impl dispatch; move the entire
FFIClientErrorCallback declaration (type alias, struct, unsafe impls, Default
impl, and impl dispatch) to below the FFIWalletEventCallbacks::dispatch impl so
the wallet struct and its methods remain contiguous, keeping the symbols
FFIWalletEventCallbacks and its dispatch impl together and relocating
FFIClientErrorCallback (including its OnClientErrorCallback, Default, unsafe
impls, and impl dispatch) after that block.
In `@dash-spv-ffi/src/client.rs`:
- Around line 406-410: The explicit drop(guard) after dispatching the error
callback is redundant because guard (from let guard =
error_callback.lock().unwrap()) will be dropped at the end of the if let Err(e)
block; remove the drop(guard) call to match the implicit-drop style used in the
start-failure path and avoid unnecessary code. Ensure you update the block
around error_callback.lock().unwrap(), cb.dispatch(&e.to_string()), and the if
let Err(e) { ... } so no explicit drop is present.
- Around line 397-410: The callback dispatch calls (cb.dispatch) occur while
holding the error_callback Mutex in both the startup error path and in
monitor_network, which can deadlock if the C on_error handler calls back into
dash_spv_ffi_client_set_client_error_callback or
dash_spv_ffi_client_clear_client_error_callback; update the safety docs for
dash_spv_ffi_client_set_client_error_callback (and mention
dash_spv_ffi_client_clear_client_error_callback) to explicitly prohibit
re-entrant calls into the callback-management API and note the Mutex/deadlock
risk, and preferably change the dispatch sites to drop the guard (release
error_callback.lock()) before calling cb.dispatch or document that callers must
not call those FFI functions from the callback (choose one approach and state it
clearly in the # Safety text).
97f4788 to
15a84d4
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
dash-spv-ffi/src/client.rs (1)
294-312:run()doc comment no longer reflects the error-reporting model.The
# Returns / 0 on success, error code on failurenote in both the Rust doc and the C header implies synchronous start-failure reporting. After this PR,run()practically always returnsFFIErrorCode::Success(only a nullclientyields a non-zero code); start failures and sync errors are delivered asynchronously viaclient_error_callback. Callers who relied on the return value to gate subsequent work will silently miss start failures.📝 Suggested doc update
/// # Returns -/// 0 on success, error code on failure. +/// 0 if background tasks were successfully spawned, non-zero on immediate failure +/// (e.g. null client pointer). +/// +/// Note: start failures and sync errors are reported **asynchronously** via the +/// client error callback set with `dash_spv_ffi_client_set_client_error_callback`. +/// Set the callback before calling `run()` to receive these notifications.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv-ffi/src/client.rs` around lines 294 - 312, The doc for run() is misleading about error reporting; update the Rust doc comment (and corresponding C header comment if generated) to state that run() returns FFIErrorCode::Success in normal cases (only a null client returns a non-zero code) and that start and runtime errors are reported asynchronously via the client_error_callback; also advise callers to register their callbacks (set_sync_event_callbacks, set_network_event_callbacks, set_wallet_event_callbacks, and client_error_callback) before calling run() so they will receive start/sync errors instead of relying on the synchronous return value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dash-spv-ffi/src/callbacks.rs`:
- Around line 589-596: FFIClientErrorCallback is missing Clone and safety
rationale comments which forces holding the Arc<Mutex<...>> lock while calling
the C callback; add #[derive(Clone)] to the FFIClientErrorCallback struct so it
can be cloned and released before dispatch (consistent with other callbacks used
by spawn_broadcast_monitor / spawn_progress_monitor) and add the same "//
SAFETY:" comment block above the unsafe impl Send and unsafe impl Sync for
FFIClientErrorCallback explaining why it is safe to send/sync (mirror the
wording used by the other callback structs) so reviewers can see the invariants
that justify the unsafe impls.
In `@dash-spv-ffi/src/client.rs`:
- Around line 385-401: The error callback mutex (client_error_callback) is held
while calling the C callback in both the spv_client.start() error branch and the
spv_client.monitor_network(...) error branch, causing a deadlock if the C
callback re-enters the FFI and tries to lock the same mutex; fix it by deriving
Clone on FFIClientErrorCallback in callbacks.rs (add #[derive(Clone)] to that
struct) and change both dispatch sites to clone the
Option<FFIClientErrorCallback> while holding the lock, drop the lock, and then
call dispatch on the cloned callback (i.e., let cb =
client_error_callback.lock().unwrap().clone(); if let Some(ref cb) = cb {
cb.dispatch(...); }) so no C callback is invoked while the mutex is held.
---
Duplicate comments:
In `@dash-spv/src/sync/sync_coordinator.rs`:
- Around line 241-243: The code reuses SyncError::InvalidState when the manager
task panics; create and use a distinct error variant (e.g.,
SyncError::ManagerPanicked or SyncError::TaskJoinError) instead of InvalidState
to avoid duplicate/ambiguous errors: add the new variant to the SyncError enum,
update its Display/From/serialization implementations as needed, and replace the
return Err(SyncError::InvalidState(...)) in the Err(e) arm inside
sync_coordinator.rs (the block that currently logs "Manager task panicked: {}")
to return the new variant with the panic details; also adjust any callers/tests
that match on InvalidState to handle the new variant.
---
Nitpick comments:
In `@dash-spv-ffi/src/client.rs`:
- Around line 294-312: The doc for run() is misleading about error reporting;
update the Rust doc comment (and corresponding C header comment if generated) to
state that run() returns FFIErrorCode::Success in normal cases (only a null
client returns a non-zero code) and that start and runtime errors are reported
asynchronously via the client_error_callback; also advise callers to register
their callbacks (set_sync_event_callbacks, set_network_event_callbacks,
set_wallet_event_callbacks, and client_error_callback) before calling run() so
they will receive start/sync errors instead of relying on the synchronous return
value.
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
xdustinface
force-pushed
the
feat/monitor-network-errors
branch
from
15a84d4 to
85fb62b
Compare
github-actions
bot
removed
the
merge-conflict
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
dash-spv-ffi/src/callbacks.rs (1)
576-617:FFIClientErrorCallbackimplementation block is placed betweenFFIWalletEventCallbacksstruct and itsimplblock.The new section (lines 576–616) separates
FFIWalletEventCallbacks's struct definition (ending ~line 574) from itsimpl FFIWalletEventCallbacksblock (line 618). Consider moving the new section either beforeFFIWalletEventCallbacksor after itsimplto keep related items contiguous.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv-ffi/src/callbacks.rs` around lines 576 - 617, The new FFIClientErrorCallback type and its impls are interrupting the contiguous definition/impl of FFIWalletEventCallbacks; move the entire FFIClientErrorCallback section (the pub type OnClientErrorCallback, struct FFIClientErrorCallback, unsafe impl Send/Sync, impl Default, and impl FFIClientErrorCallback::dispatch) so it does not sit between the FFIWalletEventCallbacks struct and its impl—either place it entirely before the FFIWalletEventCallbacks struct definition or entirely after the impl FFIWalletEventCallbacks block—preserving all function names and impl contents (OnClientErrorCallback, FFIClientErrorCallback, dispatch, Default, Send/Sync) unchanged.dash-spv-ffi/tests/unit/test_client_lifecycle.rs (1)
272-301:Arc::from_raw/Arc::into_rawborrow pattern intest_client_error_callback_dispatchis correct but fragile under concurrent dispatch.The manual borrow (
from_raw→ use →into_raw) inon_errorworks correctly here becausedispatchis called exactly once synchronously. However, if this callback were ever invoked concurrently from multiple threads (e.g., by copying the callback and callingdispatchon multiple threads), two concurrentfrom_rawcalls on the same raw pointer would be UB. A brief comment documenting the single-call assumption would improve safety clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv-ffi/tests/unit/test_client_lifecycle.rs` around lines 272 - 301, The test test_client_error_callback_dispatch uses the raw Arc borrow pattern inside extern "C" fn on_error (using StdArc::from_raw(...) then StdArc::into_raw(...)) which is only safe because dispatch is called exactly once synchronously; add a brief comment immediately above on_error (or next to FFIClientErrorCallback construction) stating the single-call / non-concurrent invocation assumption and that concurrent dispatch would cause undefined behavior due to multiple from_raw calls on the same raw pointer (reference the symbols on_error, FFIClientErrorCallback, dispatch, and error_store_raw).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dash-spv/src/client/sync_coordinator.rs`:
- Line 49: The log message formatting in tracing::info! within
sync_coordinator.rs is missing a space after the colon; update the format string
used where progress_updates is logged (the tracing::info!("Sync progress:{}",
*progress_updates.borrow()); call) to include a space ("Sync progress: {}")
while keeping the same argument (*progress_updates.borrow()) so the message
reads "Sync progress: <value>".
- Around line 36-80: run() can hang because client_task (monitor_network /
sync_coordinator tick) can return Err while shutdown_task is still blocked on
tokio::signal::ctrl_c(); to fix, capture a clone of the shutdown_token (e.g.,
shutdown_token_clone) into client_task and call shutdown_token_clone.cancel()
whenever monitor_network/tick returns an error (i.e., right before
breaking/returning Err from the client_task branch), ensuring shutdown_task
wakes and both branches can complete; update the client_task code paths that
break with Some(e) (monitor_network/tick error) to cancel the
shutdown_token_clone before exiting.
---
Duplicate comments:
In `@dash-spv-ffi/src/callbacks.rs`:
- Around line 596-597: Add a SAFETY rationale comment above the unsafe impls for
FFIClientErrorCallback: explain why instances are safe to Send and Sync (e.g.,
the struct contains only thread-safe primitives, function pointers, or FFI-safe
data and does not permit interior mutability or non-Send/Sync pointers), mirror
the phrasing used for
FFISyncEventCallbacks/FFINetworkEventCallbacks/FFIWalletEventCallbacks, and
place it immediately above the lines `unsafe impl Send for
FFIClientErrorCallback {}` and `unsafe impl Sync for FFIClientErrorCallback {}`
so the safety invariants are documented alongside the impls.
In `@dash-spv-ffi/tests/unit/test_client_lifecycle.rs`:
- Around line 246-248: Replace the fragile
thread::sleep(Duration::from_millis(200)) wait in the test with an explicit
signal from the sync task: have the sync task call start() and then send a
readiness signal (e.g. via std::sync::mpsc::Sender, a tokio::sync::oneshot, or
set an Arc<AtomicBool>) once start() completes or fails; in the test (where
thread::sleep(...) is used) wait on the corresponding Receiver or poll the
AtomicBool with a small timeout instead of sleeping. Locate the sleep call and
the sync task that invokes start() and wire a channel or Arc<AtomicBool> between
them so the test deterministically proceeds only after start() signals
readiness.
---
Nitpick comments:
In `@dash-spv-ffi/src/callbacks.rs`:
- Around line 576-617: The new FFIClientErrorCallback type and its impls are
interrupting the contiguous definition/impl of FFIWalletEventCallbacks; move the
entire FFIClientErrorCallback section (the pub type OnClientErrorCallback,
struct FFIClientErrorCallback, unsafe impl Send/Sync, impl Default, and impl
FFIClientErrorCallback::dispatch) so it does not sit between the
FFIWalletEventCallbacks struct and its impl—either place it entirely before the
FFIWalletEventCallbacks struct definition or entirely after the impl
FFIWalletEventCallbacks block—preserving all function names and impl contents
(OnClientErrorCallback, FFIClientErrorCallback, dispatch, Default, Send/Sync)
unchanged.
In `@dash-spv-ffi/tests/unit/test_client_lifecycle.rs`:
- Around line 272-301: The test test_client_error_callback_dispatch uses the raw
Arc borrow pattern inside extern "C" fn on_error (using StdArc::from_raw(...)
then StdArc::into_raw(...)) which is only safe because dispatch is called
exactly once synchronously; add a brief comment immediately above on_error (or
next to FFIClientErrorCallback construction) stating the single-call /
non-concurrent invocation assumption and that concurrent dispatch would cause
undefined behavior due to multiple from_raw calls on the same raw pointer
(reference the symbols on_error, FFIClientErrorCallback, dispatch, and
error_store_raw).
Combine `start()` + `monitor_network()` + `stop()` into a single `run(token)` entry point. Callers no longer need to call `start()` separately since `run()` handles the full lifecycle and returns after the token is cancelled. - Make `start()` an internal function - Remove `monitor_network()` its loop is now inside `run()` - Remove `dash_spv_ffi_client_start` so that `dash_spv_ffi_client_run` is the only FFI entry point - Move ctrl-c handling out of the client
We currently just silently ignore any failure inside the sync coordinator or `monitor_network` thread. This PR propagates them and also adds a FFI callback structure which can be set to receive notifications about errors inside the SPV client.
xdustinface
force-pushed
the
feat/monitor-network-errors
branch
from
85fb62b to
a138810
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We currently just silently ignore any failure inside the sync coordinator or
monitor_networkthread. This PR propagates them and also adds a FFI callback structure which can be set to receive notifications about errors inside the SPV client.Based on:
DashSpvClientto have singlerun()entry point #457Summary by CodeRabbit
New Features
Improvements
Refactor
Tests
Documentation