Introduce separate stop signal for background processor by tnull · Pull Request #622 · lightningdevkit/ldk-node

tnull · 2025-08-20T07:29:54Z

Closes#620.

Previously, we'd use the same stop signal for the background processor
as for all other background tasks which could result in the BP getting
stopped while other tasks are still produced changes that needed to be
processed before shutdown. Here we introduce a separate stop signal for
LDK's background processor, ensuring we first shutdown everything else
and disconnect all peers before finally sending the BP shutdown signal
and awaiting its shutdown.

We also remove an erroneously introduced duplicate call to chain_source.stop(), and move the remaining call slightly.

ldk-reviews-bot · 2025-08-20T07:29:56Z

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

Previously, we'd use the same stop signal for the background processor as for all other background tasks which could result in the BP getting stopped while other tasks are still produced changes that needed to be processed before shutdown. Here we introduce a separate stop signal for LDK's background processor, ensuring we first shutdown everything else and disconnect all peers before finally sending the BP shutdown signal and awaiting its shutdown.

This was introduced during a rebase. Here we simply drop the redundant call.

TheBlueMatt · 2025-08-20T14:38:34Z

 log_debug!(self.logger, "Disconnected all network peers.");

+ // Wait until non-cancellable background tasks (mod LDK's background processor) are done.
+ self.runtime.wait_on_background_tasks();


nit: This seems like we'll end up breaking it when (eventually) electrum is async and we keep the stop signal for whatever reason but its running on the background task join set. I mean we'll notice quick cause all the stops will time out and fail in tests, but just seems weird to do this before initializing stopping another thing.

Hmm, but chain_source.stop() doesn't actually stop/abort any of the running blocking tasks. So it's preferable to wait for the chain syncing to stop (or worst case timeout) before calling chain_source.stop(), which drops access to the electrum client. But, we're gonna do more refactorings soon that will hopefully allow us to get rid of the chain_source.stop() concept entirely.

tnull · 2025-08-20T07:29:54Z

Closes#620.

Previously, we'd use the same stop signal for the background processor
as for all other background tasks which could result in the BP getting
stopped while other tasks are still produced changes that needed to be
processed before shutdown. Here we introduce a separate stop signal for
LDK's background processor, ensuring we first shutdown everything else
and disconnect all peers before finally sending the BP shutdown signal
and awaiting its shutdown.

We also remove an erroneously introduced duplicate call to chain_source.stop(), and move the remaining call slightly.

ldk-reviews-bot · 2025-08-20T07:29:56Z

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

Previously, we'd use the same stop signal for the background processor as for all other background tasks which could result in the BP getting stopped while other tasks are still produced changes that needed to be processed before shutdown. Here we introduce a separate stop signal for LDK's background processor, ensuring we first shutdown everything else and disconnect all peers before finally sending the BP shutdown signal and awaiting its shutdown.

This was introduced during a rebase. Here we simply drop the redundant call.

TheBlueMatt · 2025-08-20T14:38:34Z

 log_debug!(self.logger, "Disconnected all network peers.");

+ // Wait until non-cancellable background tasks (mod LDK's background processor) are done.
+ self.runtime.wait_on_background_tasks();


nit: This seems like we'll end up breaking it when (eventually) electrum is async and we keep the stop signal for whatever reason but its running on the background task join set. I mean we'll notice quick cause all the stops will time out and fail in tests, but just seems weird to do this before initializing stopping another thing.

Hmm, but chain_source.stop() doesn't actually stop/abort any of the running blocking tasks. So it's preferable to wait for the chain syncing to stop (or worst case timeout) before calling chain_source.stop(), which drops access to the electrum client. But, we're gonna do more refactorings soon that will hopefully allow us to get rid of the chain_source.stop() concept entirely.

tnull · 2025-08-20T07:29:54Z

Closes#620.

Previously, we'd use the same stop signal for the background processor
as for all other background tasks which could result in the BP getting
stopped while other tasks are still produced changes that needed to be
processed before shutdown. Here we introduce a separate stop signal for
LDK's background processor, ensuring we first shutdown everything else
and disconnect all peers before finally sending the BP shutdown signal
and awaiting its shutdown.

We also remove an erroneously introduced duplicate call to chain_source.stop(), and move the remaining call slightly.

ldk-reviews-bot · 2025-08-20T07:29:56Z

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

Previously, we'd use the same stop signal for the background processor as for all other background tasks which could result in the BP getting stopped while other tasks are still produced changes that needed to be processed before shutdown. Here we introduce a separate stop signal for LDK's background processor, ensuring we first shutdown everything else and disconnect all peers before finally sending the BP shutdown signal and awaiting its shutdown.

This was introduced during a rebase. Here we simply drop the redundant call.

TheBlueMatt · 2025-08-20T14:38:34Z

 log_debug!(self.logger, "Disconnected all network peers.");

+ // Wait until non-cancellable background tasks (mod LDK's background processor) are done.
+ self.runtime.wait_on_background_tasks();


nit: This seems like we'll end up breaking it when (eventually) electrum is async and we keep the stop signal for whatever reason but its running on the background task join set. I mean we'll notice quick cause all the stops will time out and fail in tests, but just seems weird to do this before initializing stopping another thing.

Hmm, but chain_source.stop() doesn't actually stop/abort any of the running blocking tasks. So it's preferable to wait for the chain syncing to stop (or worst case timeout) before calling chain_source.stop(), which drops access to the electrum client. But, we're gonna do more refactorings soon that will hopefully allow us to get rid of the chain_source.stop() concept entirely.

tnull · 2025-08-20T07:29:54Z

Closes#620.

Previously, we'd use the same stop signal for the background processor
as for all other background tasks which could result in the BP getting
stopped while other tasks are still produced changes that needed to be
processed before shutdown. Here we introduce a separate stop signal for
LDK's background processor, ensuring we first shutdown everything else
and disconnect all peers before finally sending the BP shutdown signal
and awaiting its shutdown.

We also remove an erroneously introduced duplicate call to chain_source.stop(), and move the remaining call slightly.

ldk-reviews-bot · 2025-08-20T07:29:56Z

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

Previously, we'd use the same stop signal for the background processor as for all other background tasks which could result in the BP getting stopped while other tasks are still produced changes that needed to be processed before shutdown. Here we introduce a separate stop signal for LDK's background processor, ensuring we first shutdown everything else and disconnect all peers before finally sending the BP shutdown signal and awaiting its shutdown.

This was introduced during a rebase. Here we simply drop the redundant call.

TheBlueMatt · 2025-08-20T14:38:34Z

 log_debug!(self.logger, "Disconnected all network peers.");

+ // Wait until non-cancellable background tasks (mod LDK's background processor) are done.
+ self.runtime.wait_on_background_tasks();


nit: This seems like we'll end up breaking it when (eventually) electrum is async and we keep the stop signal for whatever reason but its running on the background task join set. I mean we'll notice quick cause all the stops will time out and fail in tests, but just seems weird to do this before initializing stopping another thing.

Hmm, but chain_source.stop() doesn't actually stop/abort any of the running blocking tasks. So it's preferable to wait for the chain syncing to stop (or worst case timeout) before calling chain_source.stop(), which drops access to the electrum client. But, we're gonna do more refactorings soon that will hopefully allow us to get rid of the chain_source.stop() concept entirely.

tnull · 2025-08-20T07:29:54Z

Closes#620.

Previously, we'd use the same stop signal for the background processor
as for all other background tasks which could result in the BP getting
stopped while other tasks are still produced changes that needed to be
processed before shutdown. Here we introduce a separate stop signal for
LDK's background processor, ensuring we first shutdown everything else
and disconnect all peers before finally sending the BP shutdown signal
and awaiting its shutdown.

We also remove an erroneously introduced duplicate call to chain_source.stop(), and move the remaining call slightly.

ldk-reviews-bot · 2025-08-20T07:29:56Z

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

Previously, we'd use the same stop signal for the background processor as for all other background tasks which could result in the BP getting stopped while other tasks are still produced changes that needed to be processed before shutdown. Here we introduce a separate stop signal for LDK's background processor, ensuring we first shutdown everything else and disconnect all peers before finally sending the BP shutdown signal and awaiting its shutdown.

This was introduced during a rebase. Here we simply drop the redundant call.

TheBlueMatt · 2025-08-20T14:38:34Z

 log_debug!(self.logger, "Disconnected all network peers.");

+ // Wait until non-cancellable background tasks (mod LDK's background processor) are done.
+ self.runtime.wait_on_background_tasks();


nit: This seems like we'll end up breaking it when (eventually) electrum is async and we keep the stop signal for whatever reason but its running on the background task join set. I mean we'll notice quick cause all the stops will time out and fail in tests, but just seems weird to do this before initializing stopping another thing.

Hmm, but chain_source.stop() doesn't actually stop/abort any of the running blocking tasks. So it's preferable to wait for the chain syncing to stop (or worst case timeout) before calling chain_source.stop(), which drops access to the electrum client. But, we're gonna do more refactorings soon that will hopefully allow us to get rid of the chain_source.stop() concept entirely.

tnull · 2025-08-20T07:29:54Z

Closes#620.

Previously, we'd use the same stop signal for the background processor
as for all other background tasks which could result in the BP getting
stopped while other tasks are still produced changes that needed to be
processed before shutdown. Here we introduce a separate stop signal for
LDK's background processor, ensuring we first shutdown everything else
and disconnect all peers before finally sending the BP shutdown signal
and awaiting its shutdown.

We also remove an erroneously introduced duplicate call to chain_source.stop(), and move the remaining call slightly.

ldk-reviews-bot · 2025-08-20T07:29:56Z

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

Previously, we'd use the same stop signal for the background processor as for all other background tasks which could result in the BP getting stopped while other tasks are still produced changes that needed to be processed before shutdown. Here we introduce a separate stop signal for LDK's background processor, ensuring we first shutdown everything else and disconnect all peers before finally sending the BP shutdown signal and awaiting its shutdown.

This was introduced during a rebase. Here we simply drop the redundant call.

TheBlueMatt · 2025-08-20T14:38:34Z

 log_debug!(self.logger, "Disconnected all network peers.");

+ // Wait until non-cancellable background tasks (mod LDK's background processor) are done.
+ self.runtime.wait_on_background_tasks();


nit: This seems like we'll end up breaking it when (eventually) electrum is async and we keep the stop signal for whatever reason but its running on the background task join set. I mean we'll notice quick cause all the stops will time out and fail in tests, but just seems weird to do this before initializing stopping another thing.

Hmm, but chain_source.stop() doesn't actually stop/abort any of the running blocking tasks. So it's preferable to wait for the chain syncing to stop (or worst case timeout) before calling chain_source.stop(), which drops access to the electrum client. But, we're gonna do more refactorings soon that will hopefully allow us to get rid of the chain_source.stop() concept entirely.

tnull · 2025-08-20T07:29:54Z

Closes#620.

Previously, we'd use the same stop signal for the background processor
as for all other background tasks which could result in the BP getting
stopped while other tasks are still produced changes that needed to be
processed before shutdown. Here we introduce a separate stop signal for
LDK's background processor, ensuring we first shutdown everything else
and disconnect all peers before finally sending the BP shutdown signal
and awaiting its shutdown.

We also remove an erroneously introduced duplicate call to chain_source.stop(), and move the remaining call slightly.

ldk-reviews-bot · 2025-08-20T07:29:56Z

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

Previously, we'd use the same stop signal for the background processor as for all other background tasks which could result in the BP getting stopped while other tasks are still produced changes that needed to be processed before shutdown. Here we introduce a separate stop signal for LDK's background processor, ensuring we first shutdown everything else and disconnect all peers before finally sending the BP shutdown signal and awaiting its shutdown.

This was introduced during a rebase. Here we simply drop the redundant call.

TheBlueMatt · 2025-08-20T14:38:34Z

 log_debug!(self.logger, "Disconnected all network peers.");

+ // Wait until non-cancellable background tasks (mod LDK's background processor) are done.
+ self.runtime.wait_on_background_tasks();


nit: This seems like we'll end up breaking it when (eventually) electrum is async and we keep the stop signal for whatever reason but its running on the background task join set. I mean we'll notice quick cause all the stops will time out and fail in tests, but just seems weird to do this before initializing stopping another thing.

Hmm, but chain_source.stop() doesn't actually stop/abort any of the running blocking tasks. So it's preferable to wait for the chain syncing to stop (or worst case timeout) before calling chain_source.stop(), which drops access to the electrum client. But, we're gonna do more refactorings soon that will hopefully allow us to get rid of the chain_source.stop() concept entirely.

tnull · 2025-08-20T07:29:54Z

Closes#620.

Previously, we'd use the same stop signal for the background processor
as for all other background tasks which could result in the BP getting
stopped while other tasks are still produced changes that needed to be
processed before shutdown. Here we introduce a separate stop signal for
LDK's background processor, ensuring we first shutdown everything else
and disconnect all peers before finally sending the BP shutdown signal
and awaiting its shutdown.

We also remove an erroneously introduced duplicate call to chain_source.stop(), and move the remaining call slightly.

ldk-reviews-bot · 2025-08-20T07:29:56Z

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

Previously, we'd use the same stop signal for the background processor as for all other background tasks which could result in the BP getting stopped while other tasks are still produced changes that needed to be processed before shutdown. Here we introduce a separate stop signal for LDK's background processor, ensuring we first shutdown everything else and disconnect all peers before finally sending the BP shutdown signal and awaiting its shutdown.

This was introduced during a rebase. Here we simply drop the redundant call.

TheBlueMatt · 2025-08-20T14:38:34Z

 log_debug!(self.logger, "Disconnected all network peers.");

+ // Wait until non-cancellable background tasks (mod LDK's background processor) are done.
+ self.runtime.wait_on_background_tasks();


nit: This seems like we'll end up breaking it when (eventually) electrum is async and we keep the stop signal for whatever reason but its running on the background task join set. I mean we'll notice quick cause all the stops will time out and fail in tests, but just seems weird to do this before initializing stopping another thing.

Hmm, but chain_source.stop() doesn't actually stop/abort any of the running blocking tasks. So it's preferable to wait for the chain syncing to stop (or worst case timeout) before calling chain_source.stop(), which drops access to the electrum client. But, we're gonna do more refactorings soon that will hopefully allow us to get rid of the chain_source.stop() concept entirely.

tnull requested a review from TheBlueMatt August 20, 2025 07:29

tnull added 2 commits August 20, 2025 09:33