Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 64 additions & 16 deletions lightning-background-processor/src/lib.rs
Original file line numberDiff line numberDiff line change
Expand Up@@ -41,9 +41,7 @@ use std::ops::Deref;
/// for unilateral chain closure fees are at risk.
pub struct BackgroundProcessor {
stop_thread: Arc<AtomicBool>,
/// May be used to retrieve and handle the error if `BackgroundProcessor`'s thread
/// exits due to an error while persisting.
pub thread_handle: JoinHandle<Result<(), std::io::Error>>,
thread_handle: Option<JoinHandle<Result<(), std::io::Error>>>,
}

#[cfg(not(test))]
Expand DownExpand Up@@ -84,21 +82,25 @@ ChannelManagerPersister<Signer, M, T, K, F, L> for Fun where
}

impl BackgroundProcessor {
/// Start a background thread that takes care of responsibilities enumerated in the top-level
/// documentation.
/// Start a background thread that takes care of responsibilities enumerated in the [top-level
/// documentation].
///
/// If `persist_manager` returns an error, then this thread will return said error (and
/// `start()` will need to be called again to restart the `BackgroundProcessor`). Users should
/// wait on [`thread_handle`]'s `join()` method to be able to tell if and when an error is
/// returned, or implement `persist_manager` such that an error is never returned to the
/// `BackgroundProcessor`
/// The thread runs indefinitely unless the object is dropped, [`stop`] is called, or
/// `persist_manager` returns an error. In case of an error, the error is retrieved by calling
/// either [`join`] or [`stop`].
///
/// Typically, users should either implement [`ChannelManagerPersister`] to never return an
/// error or call [`join`] and handle any error that may arise. For the latter case, the
/// `BackgroundProcessor` must be restarted by calling `start` again after handling the error.
///
/// `persist_manager` is responsible for writing out the [`ChannelManager`] to disk, and/or
/// uploading to one or more backup services. See [`ChannelManager::write`] for writing out a
/// [`ChannelManager`]. See [`FilesystemPersister::persist_manager`] for Rust-Lightning's
/// provided implementation.
///
/// [`thread_handle`]: BackgroundProcessor::thread_handle
/// [top-level documentation]: Self
/// [`join`]: Self::join
/// [`stop`]: Self::stop
/// [`ChannelManager`]: lightning::ln::channelmanager::ChannelManager
/// [`ChannelManager::write`]: lightning::ln::channelmanager::ChannelManager#impl-Writeable
/// [`FilesystemPersister::persist_manager`]: lightning_persister::FilesystemPersister::persist_manager
Expand DownExpand Up@@ -158,13 +160,53 @@ impl BackgroundProcessor {
}
}
});
Self { stop_thread: stop_thread_clone, thread_handle: handle }
Self { stop_thread: stop_thread_clone, thread_handle: Some(handle) }
}

/// Join `BackgroundProcessor`'s thread, returning any error that occurred while persisting
/// [`ChannelManager`].
///
/// # Panics
///
/// This function panics if the background thread has panicked such as while persisting or
/// handling events.
///
/// [`ChannelManager`]: lightning::ln::channelmanager::ChannelManager
pub fn join(mut self) -> Result<(), std::io::Error> {
assert!(self.thread_handle.is_some());
self.join_thread()
}

/// Stop `BackgroundProcessor`'s thread, returning any error that occurred while persisting
/// [`ChannelManager`].
///
/// # Panics
///
/// This function panics if the background thread has panicked such as while persisting or
/// handling events.
///
/// [`ChannelManager`]: lightning::ln::channelmanager::ChannelManager
pub fn stop(mut self) -> Result<(), std::io::Error> {
assert!(self.thread_handle.is_some());
Comment thread
valentinewallace marked this conversation as resolved.
self.stop_and_join_thread()
}

/// Stop `BackgroundProcessor`'s thread.
pub fn stop(self) -> Result<(), std::io::Error> {
fn stop_and_join_thread(&mut self) -> Result<(), std::io::Error> {
self.stop_thread.store(true, Ordering::Release);
self.thread_handle.join().unwrap()
self.join_thread()
}

fn join_thread(&mut self) -> Result<(), std::io::Error> {
match self.thread_handle.take() {
Some(handle) => handle.join().unwrap(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that if the background thread panics, join will return an error that will cause the unwrap to panic on this line. Might it'd be preferable to either return the panic as an error or at least document that this is the expected behavior?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added docs about panicking to join and stop.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does feel a bit strange that we have a function that returns a Result, but instead of ever returning an Err it always panics.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, so the behavior of std's join() is to return an error if the thread panics. Might it make more sense for our join method to mirror that behavior?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does feel a bit strange that we have a function that returns a Result, but instead of ever returning an Err it always panics.

It will return an Err if the persister returns an Err. The unwrap is to remove the error handling JoinHandle provides for panics, not for the Result returned by the thread's closure.

Hmm, so the behavior of std's join() is to return an error if the thread panics. Might it make more sense for our join method to mirror that behavior?

My intention was to mirror the same behavior as stop, which would already panic in this case. The reasoning for panicking was because the panic would likely have originated from user code. So users should either write code to not panic in the case of event handling (some discussion here) or to return an error in case of persisting. If the panic is coming from RL, my assumption was we'd want to propagate it.

I'm not necessarily opposed to making it return the wrapped result, but I think we'd want stop to have the same behavior. There may be a good argument based on how a user configures Rust to handle panics. I vaguely remember @TheBlueMatt discussing it with us but I'm a bit foggy on the details.

None => Ok(()),
}
}
}

impl Drop for BackgroundProcessor {
fn drop(&mut self) {
self.stop_and_join_thread().unwrap();
}
}

Expand DownExpand Up@@ -416,7 +458,13 @@ mod tests {
let persister = |_: &_| Err(std::io::Error::new(std::io::ErrorKind::Other, "test"));
let event_handler = |_| {};
let bg_processor = BackgroundProcessor::start(persister, event_handler, nodes[0].chain_monitor.clone(), nodes[0].node.clone(), nodes[0].peer_manager.clone(), nodes[0].logger.clone());
let _ = bg_processor.thread_handle.join().unwrap().expect_err("Errored persisting manager: test");
match bg_processor.join() {
Ok(_) => panic!("Expected error persisting manager"),
Err(e) => {
assert_eq!(e.kind(), std::io::ErrorKind::Other);
assert_eq!(e.get_ref().unwrap().to_string(), "test");
},
}
}

#[test]
Expand Down
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 64 additions & 16 deletions lightning-background-processor/src/lib.rs
Original file line numberDiff line numberDiff line change
Expand Up@@ -41,9 +41,7 @@ use std::ops::Deref;
/// for unilateral chain closure fees are at risk.
pub struct BackgroundProcessor {
stop_thread: Arc<AtomicBool>,
/// May be used to retrieve and handle the error if `BackgroundProcessor`'s thread
/// exits due to an error while persisting.
pub thread_handle: JoinHandle<Result<(), std::io::Error>>,
thread_handle: Option<JoinHandle<Result<(), std::io::Error>>>,
}

#[cfg(not(test))]
Expand DownExpand Up@@ -84,21 +82,25 @@ ChannelManagerPersister<Signer, M, T, K, F, L> for Fun where
}

impl BackgroundProcessor {
/// Start a background thread that takes care of responsibilities enumerated in the top-level
/// documentation.
/// Start a background thread that takes care of responsibilities enumerated in the [top-level
/// documentation].
///
/// If `persist_manager` returns an error, then this thread will return said error (and
/// `start()` will need to be called again to restart the `BackgroundProcessor`). Users should
/// wait on [`thread_handle`]'s `join()` method to be able to tell if and when an error is
/// returned, or implement `persist_manager` such that an error is never returned to the
/// `BackgroundProcessor`
/// The thread runs indefinitely unless the object is dropped, [`stop`] is called, or
/// `persist_manager` returns an error. In case of an error, the error is retrieved by calling
/// either [`join`] or [`stop`].
///
/// Typically, users should either implement [`ChannelManagerPersister`] to never return an
/// error or call [`join`] and handle any error that may arise. For the latter case, the
/// `BackgroundProcessor` must be restarted by calling `start` again after handling the error.
///
/// `persist_manager` is responsible for writing out the [`ChannelManager`] to disk, and/or
/// uploading to one or more backup services. See [`ChannelManager::write`] for writing out a
/// [`ChannelManager`]. See [`FilesystemPersister::persist_manager`] for Rust-Lightning's
/// provided implementation.
///
/// [`thread_handle`]: BackgroundProcessor::thread_handle
/// [top-level documentation]: Self
/// [`join`]: Self::join
/// [`stop`]: Self::stop
/// [`ChannelManager`]: lightning::ln::channelmanager::ChannelManager
/// [`ChannelManager::write`]: lightning::ln::channelmanager::ChannelManager#impl-Writeable
/// [`FilesystemPersister::persist_manager`]: lightning_persister::FilesystemPersister::persist_manager
Expand DownExpand Up@@ -158,13 +160,53 @@ impl BackgroundProcessor {
}
}
});
Self { stop_thread: stop_thread_clone, thread_handle: handle }
Self { stop_thread: stop_thread_clone, thread_handle: Some(handle) }
}

/// Join `BackgroundProcessor`'s thread, returning any error that occurred while persisting
/// [`ChannelManager`].
///
/// # Panics
///
/// This function panics if the background thread has panicked such as while persisting or
/// handling events.
///
/// [`ChannelManager`]: lightning::ln::channelmanager::ChannelManager
pub fn join(mut self) -> Result<(), std::io::Error> {
assert!(self.thread_handle.is_some());
self.join_thread()
}

/// Stop `BackgroundProcessor`'s thread, returning any error that occurred while persisting
/// [`ChannelManager`].
///
/// # Panics
///
/// This function panics if the background thread has panicked such as while persisting or
/// handling events.
///
/// [`ChannelManager`]: lightning::ln::channelmanager::ChannelManager
pub fn stop(mut self) -> Result<(), std::io::Error> {
assert!(self.thread_handle.is_some());
Comment thread
valentinewallace marked this conversation as resolved.
self.stop_and_join_thread()
}

/// Stop `BackgroundProcessor`'s thread.
pub fn stop(self) -> Result<(), std::io::Error> {
fn stop_and_join_thread(&mut self) -> Result<(), std::io::Error> {
self.stop_thread.store(true, Ordering::Release);
self.thread_handle.join().unwrap()
self.join_thread()
}

fn join_thread(&mut self) -> Result<(), std::io::Error> {
match self.thread_handle.take() {
Some(handle) => handle.join().unwrap(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that if the background thread panics, join will return an error that will cause the unwrap to panic on this line. Might it'd be preferable to either return the panic as an error or at least document that this is the expected behavior?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added docs about panicking to join and stop.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does feel a bit strange that we have a function that returns a Result, but instead of ever returning an Err it always panics.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, so the behavior of std's join() is to return an error if the thread panics. Might it make more sense for our join method to mirror that behavior?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does feel a bit strange that we have a function that returns a Result, but instead of ever returning an Err it always panics.

It will return an Err if the persister returns an Err. The unwrap is to remove the error handling JoinHandle provides for panics, not for the Result returned by the thread's closure.

Hmm, so the behavior of std's join() is to return an error if the thread panics. Might it make more sense for our join method to mirror that behavior?

My intention was to mirror the same behavior as stop, which would already panic in this case. The reasoning for panicking was because the panic would likely have originated from user code. So users should either write code to not panic in the case of event handling (some discussion here) or to return an error in case of persisting. If the panic is coming from RL, my assumption was we'd want to propagate it.

I'm not necessarily opposed to making it return the wrapped result, but I think we'd want stop to have the same behavior. There may be a good argument based on how a user configures Rust to handle panics. I vaguely remember @TheBlueMatt discussing it with us but I'm a bit foggy on the details.

None => Ok(()),
}
}
}

impl Drop for BackgroundProcessor {
fn drop(&mut self) {
self.stop_and_join_thread().unwrap();
}
}

Expand DownExpand Up@@ -416,7 +458,13 @@ mod tests {
let persister = |_: &_| Err(std::io::Error::new(std::io::ErrorKind::Other, "test"));
let event_handler = |_| {};
let bg_processor = BackgroundProcessor::start(persister, event_handler, nodes[0].chain_monitor.clone(), nodes[0].node.clone(), nodes[0].peer_manager.clone(), nodes[0].logger.clone());
let _ = bg_processor.thread_handle.join().unwrap().expect_err("Errored persisting manager: test");
match bg_processor.join() {
Ok(_) => panic!("Expected error persisting manager"),
Err(e) => {
assert_eq!(e.kind(), std::io::ErrorKind::Other);
assert_eq!(e.get_ref().unwrap().to_string(), "test");
},
}
}

#[test]
Expand Down
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 64 additions & 16 deletions lightning-background-processor/src/lib.rs
Original file line numberDiff line numberDiff line change
Expand Up@@ -41,9 +41,7 @@ use std::ops::Deref;
/// for unilateral chain closure fees are at risk.
pub struct BackgroundProcessor {
stop_thread: Arc<AtomicBool>,
/// May be used to retrieve and handle the error if `BackgroundProcessor`'s thread
/// exits due to an error while persisting.
pub thread_handle: JoinHandle<Result<(), std::io::Error>>,
thread_handle: Option<JoinHandle<Result<(), std::io::Error>>>,
}

#[cfg(not(test))]
Expand DownExpand Up@@ -84,21 +82,25 @@ ChannelManagerPersister<Signer, M, T, K, F, L> for Fun where
}

impl BackgroundProcessor {
/// Start a background thread that takes care of responsibilities enumerated in the top-level
/// documentation.
/// Start a background thread that takes care of responsibilities enumerated in the [top-level
/// documentation].
///
/// If `persist_manager` returns an error, then this thread will return said error (and
/// `start()` will need to be called again to restart the `BackgroundProcessor`). Users should
/// wait on [`thread_handle`]'s `join()` method to be able to tell if and when an error is
/// returned, or implement `persist_manager` such that an error is never returned to the
/// `BackgroundProcessor`
/// The thread runs indefinitely unless the object is dropped, [`stop`] is called, or
/// `persist_manager` returns an error. In case of an error, the error is retrieved by calling
/// either [`join`] or [`stop`].
///
/// Typically, users should either implement [`ChannelManagerPersister`] to never return an
/// error or call [`join`] and handle any error that may arise. For the latter case, the
/// `BackgroundProcessor` must be restarted by calling `start` again after handling the error.
///
/// `persist_manager` is responsible for writing out the [`ChannelManager`] to disk, and/or
/// uploading to one or more backup services. See [`ChannelManager::write`] for writing out a
/// [`ChannelManager`]. See [`FilesystemPersister::persist_manager`] for Rust-Lightning's
/// provided implementation.
///
/// [`thread_handle`]: BackgroundProcessor::thread_handle
/// [top-level documentation]: Self
/// [`join`]: Self::join
/// [`stop`]: Self::stop
/// [`ChannelManager`]: lightning::ln::channelmanager::ChannelManager
/// [`ChannelManager::write`]: lightning::ln::channelmanager::ChannelManager#impl-Writeable
/// [`FilesystemPersister::persist_manager`]: lightning_persister::FilesystemPersister::persist_manager
Expand DownExpand Up@@ -158,13 +160,53 @@ impl BackgroundProcessor {
}
}
});
Self { stop_thread: stop_thread_clone, thread_handle: handle }
Self { stop_thread: stop_thread_clone, thread_handle: Some(handle) }
}

/// Join `BackgroundProcessor`'s thread, returning any error that occurred while persisting
/// [`ChannelManager`].
///
/// # Panics
///
/// This function panics if the background thread has panicked such as while persisting or
/// handling events.
///
/// [`ChannelManager`]: lightning::ln::channelmanager::ChannelManager
pub fn join(mut self) -> Result<(), std::io::Error> {
assert!(self.thread_handle.is_some());
self.join_thread()
}

/// Stop `BackgroundProcessor`'s thread, returning any error that occurred while persisting
/// [`ChannelManager`].
///
/// # Panics
///
/// This function panics if the background thread has panicked such as while persisting or
/// handling events.
///
/// [`ChannelManager`]: lightning::ln::channelmanager::ChannelManager
pub fn stop(mut self) -> Result<(), std::io::Error> {
assert!(self.thread_handle.is_some());
Comment thread
valentinewallace marked this conversation as resolved.
self.stop_and_join_thread()
}

/// Stop `BackgroundProcessor`'s thread.
pub fn stop(self) -> Result<(), std::io::Error> {
fn stop_and_join_thread(&mut self) -> Result<(), std::io::Error> {
self.stop_thread.store(true, Ordering::Release);
self.thread_handle.join().unwrap()
self.join_thread()
}

fn join_thread(&mut self) -> Result<(), std::io::Error> {
match self.thread_handle.take() {
Some(handle) => handle.join().unwrap(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that if the background thread panics, join will return an error that will cause the unwrap to panic on this line. Might it'd be preferable to either return the panic as an error or at least document that this is the expected behavior?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added docs about panicking to join and stop.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does feel a bit strange that we have a function that returns a Result, but instead of ever returning an Err it always panics.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, so the behavior of std's join() is to return an error if the thread panics. Might it make more sense for our join method to mirror that behavior?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does feel a bit strange that we have a function that returns a Result, but instead of ever returning an Err it always panics.

It will return an Err if the persister returns an Err. The unwrap is to remove the error handling JoinHandle provides for panics, not for the Result returned by the thread's closure.

Hmm, so the behavior of std's join() is to return an error if the thread panics. Might it make more sense for our join method to mirror that behavior?

My intention was to mirror the same behavior as stop, which would already panic in this case. The reasoning for panicking was because the panic would likely have originated from user code. So users should either write code to not panic in the case of event handling (some discussion here) or to return an error in case of persisting. If the panic is coming from RL, my assumption was we'd want to propagate it.

I'm not necessarily opposed to making it return the wrapped result, but I think we'd want stop to have the same behavior. There may be a good argument based on how a user configures Rust to handle panics. I vaguely remember @TheBlueMatt discussing it with us but I'm a bit foggy on the details.

None => Ok(()),
}
}
}

impl Drop for BackgroundProcessor {
fn drop(&mut self) {
self.stop_and_join_thread().unwrap();
}
}

Expand DownExpand Up@@ -416,7 +458,13 @@ mod tests {
let persister = |_: &_| Err(std::io::Error::new(std::io::ErrorKind::Other, "test"));
let event_handler = |_| {};
let bg_processor = BackgroundProcessor::start(persister, event_handler, nodes[0].chain_monitor.clone(), nodes[0].node.clone(), nodes[0].peer_manager.clone(), nodes[0].logger.clone());
let _ = bg_processor.thread_handle.join().unwrap().expect_err("Errored persisting manager: test");
match bg_processor.join() {
Ok(_) => panic!("Expected error persisting manager"),
Err(e) => {
assert_eq!(e.kind(), std::io::ErrorKind::Other);
assert_eq!(e.get_ref().unwrap().to_string(), "test");
},
}
}

#[test]
Expand Down
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 64 additions & 16 deletions lightning-background-processor/src/lib.rs
Original file line numberDiff line numberDiff line change
Expand Up@@ -41,9 +41,7 @@ use std::ops::Deref;
/// for unilateral chain closure fees are at risk.
pub struct BackgroundProcessor {
stop_thread: Arc<AtomicBool>,
/// May be used to retrieve and handle the error if `BackgroundProcessor`'s thread
/// exits due to an error while persisting.
pub thread_handle: JoinHandle<Result<(), std::io::Error>>,
thread_handle: Option<JoinHandle<Result<(), std::io::Error>>>,
}

#[cfg(not(test))]
Expand DownExpand Up@@ -84,21 +82,25 @@ ChannelManagerPersister<Signer, M, T, K, F, L> for Fun where
}

impl BackgroundProcessor {
/// Start a background thread that takes care of responsibilities enumerated in the top-level
/// documentation.
/// Start a background thread that takes care of responsibilities enumerated in the [top-level
/// documentation].
///
/// If `persist_manager` returns an error, then this thread will return said error (and
/// `start()` will need to be called again to restart the `BackgroundProcessor`). Users should
/// wait on [`thread_handle`]'s `join()` method to be able to tell if and when an error is
/// returned, or implement `persist_manager` such that an error is never returned to the
/// `BackgroundProcessor`
/// The thread runs indefinitely unless the object is dropped, [`stop`] is called, or
/// `persist_manager` returns an error. In case of an error, the error is retrieved by calling
/// either [`join`] or [`stop`].
///
/// Typically, users should either implement [`ChannelManagerPersister`] to never return an
/// error or call [`join`] and handle any error that may arise. For the latter case, the
/// `BackgroundProcessor` must be restarted by calling `start` again after handling the error.
///
/// `persist_manager` is responsible for writing out the [`ChannelManager`] to disk, and/or
/// uploading to one or more backup services. See [`ChannelManager::write`] for writing out a
/// [`ChannelManager`]. See [`FilesystemPersister::persist_manager`] for Rust-Lightning's
/// provided implementation.
///
/// [`thread_handle`]: BackgroundProcessor::thread_handle
/// [top-level documentation]: Self
/// [`join`]: Self::join
/// [`stop`]: Self::stop
/// [`ChannelManager`]: lightning::ln::channelmanager::ChannelManager
/// [`ChannelManager::write`]: lightning::ln::channelmanager::ChannelManager#impl-Writeable
/// [`FilesystemPersister::persist_manager`]: lightning_persister::FilesystemPersister::persist_manager
Expand DownExpand Up@@ -158,13 +160,53 @@ impl BackgroundProcessor {
}
}
});
Self { stop_thread: stop_thread_clone, thread_handle: handle }
Self { stop_thread: stop_thread_clone, thread_handle: Some(handle) }
}

/// Join `BackgroundProcessor`'s thread, returning any error that occurred while persisting
/// [`ChannelManager`].
///
/// # Panics
///
/// This function panics if the background thread has panicked such as while persisting or
/// handling events.
///
/// [`ChannelManager`]: lightning::ln::channelmanager::ChannelManager
pub fn join(mut self) -> Result<(), std::io::Error> {
assert!(self.thread_handle.is_some());
self.join_thread()
}

/// Stop `BackgroundProcessor`'s thread, returning any error that occurred while persisting
/// [`ChannelManager`].
///
/// # Panics
///
/// This function panics if the background thread has panicked such as while persisting or
/// handling events.
///
/// [`ChannelManager`]: lightning::ln::channelmanager::ChannelManager
pub fn stop(mut self) -> Result<(), std::io::Error> {
assert!(self.thread_handle.is_some());
Comment thread
valentinewallace marked this conversation as resolved.
self.stop_and_join_thread()
}

/// Stop `BackgroundProcessor`'s thread.
pub fn stop(self) -> Result<(), std::io::Error> {
fn stop_and_join_thread(&mut self) -> Result<(), std::io::Error> {
self.stop_thread.store(true, Ordering::Release);
self.thread_handle.join().unwrap()
self.join_thread()
}

fn join_thread(&mut self) -> Result<(), std::io::Error> {
match self.thread_handle.take() {
Some(handle) => handle.join().unwrap(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that if the background thread panics, join will return an error that will cause the unwrap to panic on this line. Might it'd be preferable to either return the panic as an error or at least document that this is the expected behavior?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added docs about panicking to join and stop.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does feel a bit strange that we have a function that returns a Result, but instead of ever returning an Err it always panics.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, so the behavior of std's join() is to return an error if the thread panics. Might it make more sense for our join method to mirror that behavior?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does feel a bit strange that we have a function that returns a Result, but instead of ever returning an Err it always panics.

It will return an Err if the persister returns an Err. The unwrap is to remove the error handling JoinHandle provides for panics, not for the Result returned by the thread's closure.

Hmm, so the behavior of std's join() is to return an error if the thread panics. Might it make more sense for our join method to mirror that behavior?

My intention was to mirror the same behavior as stop, which would already panic in this case. The reasoning for panicking was because the panic would likely have originated from user code. So users should either write code to not panic in the case of event handling (some discussion here) or to return an error in case of persisting. If the panic is coming from RL, my assumption was we'd want to propagate it.

I'm not necessarily opposed to making it return the wrapped result, but I think we'd want stop to have the same behavior. There may be a good argument based on how a user configures Rust to handle panics. I vaguely remember @TheBlueMatt discussing it with us but I'm a bit foggy on the details.

None => Ok(()),
}
}
}

impl Drop for BackgroundProcessor {
fn drop(&mut self) {
self.stop_and_join_thread().unwrap();
}
}

Expand DownExpand Up@@ -416,7 +458,13 @@ mod tests {
let persister = |_: &_| Err(std::io::Error::new(std::io::ErrorKind::Other, "test"));
let event_handler = |_| {};
let bg_processor = BackgroundProcessor::start(persister, event_handler, nodes[0].chain_monitor.clone(), nodes[0].node.clone(), nodes[0].peer_manager.clone(), nodes[0].logger.clone());
let _ = bg_processor.thread_handle.join().unwrap().expect_err("Errored persisting manager: test");
match bg_processor.join() {
Ok(_) => panic!("Expected error persisting manager"),
Err(e) => {
assert_eq!(e.kind(), std::io::ErrorKind::Other);
assert_eq!(e.get_ref().unwrap().to_string(), "test");
},
}
}

#[test]
Expand Down
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 64 additions & 16 deletions lightning-background-processor/src/lib.rs
Original file line numberDiff line numberDiff line change
Expand Up@@ -41,9 +41,7 @@ use std::ops::Deref;
/// for unilateral chain closure fees are at risk.
pub struct BackgroundProcessor {
stop_thread: Arc<AtomicBool>,
/// May be used to retrieve and handle the error if `BackgroundProcessor`'s thread
/// exits due to an error while persisting.
pub thread_handle: JoinHandle<Result<(), std::io::Error>>,
thread_handle: Option<JoinHandle<Result<(), std::io::Error>>>,
}

#[cfg(not(test))]
Expand DownExpand Up@@ -84,21 +82,25 @@ ChannelManagerPersister<Signer, M, T, K, F, L> for Fun where
}

impl BackgroundProcessor {
/// Start a background thread that takes care of responsibilities enumerated in the top-level
/// documentation.
/// Start a background thread that takes care of responsibilities enumerated in the [top-level
/// documentation].
///
/// If `persist_manager` returns an error, then this thread will return said error (and
/// `start()` will need to be called again to restart the `BackgroundProcessor`). Users should
/// wait on [`thread_handle`]'s `join()` method to be able to tell if and when an error is
/// returned, or implement `persist_manager` such that an error is never returned to the
/// `BackgroundProcessor`
/// The thread runs indefinitely unless the object is dropped, [`stop`] is called, or
/// `persist_manager` returns an error. In case of an error, the error is retrieved by calling
/// either [`join`] or [`stop`].
///
/// Typically, users should either implement [`ChannelManagerPersister`] to never return an
/// error or call [`join`] and handle any error that may arise. For the latter case, the
/// `BackgroundProcessor` must be restarted by calling `start` again after handling the error.
///
/// `persist_manager` is responsible for writing out the [`ChannelManager`] to disk, and/or
/// uploading to one or more backup services. See [`ChannelManager::write`] for writing out a
/// [`ChannelManager`]. See [`FilesystemPersister::persist_manager`] for Rust-Lightning's
/// provided implementation.
///
/// [`thread_handle`]: BackgroundProcessor::thread_handle
/// [top-level documentation]: Self
/// [`join`]: Self::join
/// [`stop`]: Self::stop
/// [`ChannelManager`]: lightning::ln::channelmanager::ChannelManager
/// [`ChannelManager::write`]: lightning::ln::channelmanager::ChannelManager#impl-Writeable
/// [`FilesystemPersister::persist_manager`]: lightning_persister::FilesystemPersister::persist_manager
Expand DownExpand Up@@ -158,13 +160,53 @@ impl BackgroundProcessor {
}
}
});
Self { stop_thread: stop_thread_clone, thread_handle: handle }
Self { stop_thread: stop_thread_clone, thread_handle: Some(handle) }
}

/// Join `BackgroundProcessor`'s thread, returning any error that occurred while persisting
/// [`ChannelManager`].
///
/// # Panics
///
/// This function panics if the background thread has panicked such as while persisting or
/// handling events.
///
/// [`ChannelManager`]: lightning::ln::channelmanager::ChannelManager
pub fn join(mut self) -> Result<(), std::io::Error> {
assert!(self.thread_handle.is_some());
self.join_thread()
}

/// Stop `BackgroundProcessor`'s thread, returning any error that occurred while persisting
/// [`ChannelManager`].
///
/// # Panics
///
/// This function panics if the background thread has panicked such as while persisting or
/// handling events.
///
/// [`ChannelManager`]: lightning::ln::channelmanager::ChannelManager
pub fn stop(mut self) -> Result<(), std::io::Error> {
assert!(self.thread_handle.is_some());
Comment thread
valentinewallace marked this conversation as resolved.
self.stop_and_join_thread()
}

/// Stop `BackgroundProcessor`'s thread.
pub fn stop(self) -> Result<(), std::io::Error> {
fn stop_and_join_thread(&mut self) -> Result<(), std::io::Error> {
self.stop_thread.store(true, Ordering::Release);
self.thread_handle.join().unwrap()
self.join_thread()
}

fn join_thread(&mut self) -> Result<(), std::io::Error> {
match self.thread_handle.take() {
Some(handle) => handle.join().unwrap(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that if the background thread panics, join will return an error that will cause the unwrap to panic on this line. Might it'd be preferable to either return the panic as an error or at least document that this is the expected behavior?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added docs about panicking to join and stop.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does feel a bit strange that we have a function that returns a Result, but instead of ever returning an Err it always panics.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, so the behavior of std's join() is to return an error if the thread panics. Might it make more sense for our join method to mirror that behavior?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does feel a bit strange that we have a function that returns a Result, but instead of ever returning an Err it always panics.

It will return an Err if the persister returns an Err. The unwrap is to remove the error handling JoinHandle provides for panics, not for the Result returned by the thread's closure.

Hmm, so the behavior of std's join() is to return an error if the thread panics. Might it make more sense for our join method to mirror that behavior?

My intention was to mirror the same behavior as stop, which would already panic in this case. The reasoning for panicking was because the panic would likely have originated from user code. So users should either write code to not panic in the case of event handling (some discussion here) or to return an error in case of persisting. If the panic is coming from RL, my assumption was we'd want to propagate it.

I'm not necessarily opposed to making it return the wrapped result, but I think we'd want stop to have the same behavior. There may be a good argument based on how a user configures Rust to handle panics. I vaguely remember @TheBlueMatt discussing it with us but I'm a bit foggy on the details.

None => Ok(()),
}
}
}

impl Drop for BackgroundProcessor {
fn drop(&mut self) {
self.stop_and_join_thread().unwrap();
}
}

Expand DownExpand Up@@ -416,7 +458,13 @@ mod tests {
let persister = |_: &_| Err(std::io::Error::new(std::io::ErrorKind::Other, "test"));
let event_handler = |_| {};
let bg_processor = BackgroundProcessor::start(persister, event_handler, nodes[0].chain_monitor.clone(), nodes[0].node.clone(), nodes[0].peer_manager.clone(), nodes[0].logger.clone());
let _ = bg_processor.thread_handle.join().unwrap().expect_err("Errored persisting manager: test");
match bg_processor.join() {
Ok(_) => panic!("Expected error persisting manager"),
Err(e) => {
assert_eq!(e.kind(), std::io::ErrorKind::Other);
assert_eq!(e.get_ref().unwrap().to_string(), "test");
},
}
}

#[test]
Expand Down
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 64 additions & 16 deletions lightning-background-processor/src/lib.rs
Original file line numberDiff line numberDiff line change
Expand Up@@ -41,9 +41,7 @@ use std::ops::Deref;
/// for unilateral chain closure fees are at risk.
pub struct BackgroundProcessor {
stop_thread: Arc<AtomicBool>,
/// May be used to retrieve and handle the error if `BackgroundProcessor`'s thread
/// exits due to an error while persisting.
pub thread_handle: JoinHandle<Result<(), std::io::Error>>,
thread_handle: Option<JoinHandle<Result<(), std::io::Error>>>,
}

#[cfg(not(test))]
Expand DownExpand Up@@ -84,21 +82,25 @@ ChannelManagerPersister<Signer, M, T, K, F, L> for Fun where
}

impl BackgroundProcessor {
/// Start a background thread that takes care of responsibilities enumerated in the top-level
/// documentation.
/// Start a background thread that takes care of responsibilities enumerated in the [top-level
/// documentation].
///
/// If `persist_manager` returns an error, then this thread will return said error (and
/// `start()` will need to be called again to restart the `BackgroundProcessor`). Users should
/// wait on [`thread_handle`]'s `join()` method to be able to tell if and when an error is
/// returned, or implement `persist_manager` such that an error is never returned to the
/// `BackgroundProcessor`
/// The thread runs indefinitely unless the object is dropped, [`stop`] is called, or
/// `persist_manager` returns an error. In case of an error, the error is retrieved by calling
/// either [`join`] or [`stop`].
///
/// Typically, users should either implement [`ChannelManagerPersister`] to never return an
/// error or call [`join`] and handle any error that may arise. For the latter case, the
/// `BackgroundProcessor` must be restarted by calling `start` again after handling the error.
///
/// `persist_manager` is responsible for writing out the [`ChannelManager`] to disk, and/or
/// uploading to one or more backup services. See [`ChannelManager::write`] for writing out a
/// [`ChannelManager`]. See [`FilesystemPersister::persist_manager`] for Rust-Lightning's
/// provided implementation.
///
/// [`thread_handle`]: BackgroundProcessor::thread_handle
/// [top-level documentation]: Self
/// [`join`]: Self::join
/// [`stop`]: Self::stop
/// [`ChannelManager`]: lightning::ln::channelmanager::ChannelManager
/// [`ChannelManager::write`]: lightning::ln::channelmanager::ChannelManager#impl-Writeable
/// [`FilesystemPersister::persist_manager`]: lightning_persister::FilesystemPersister::persist_manager
Expand DownExpand Up@@ -158,13 +160,53 @@ impl BackgroundProcessor {
}
}
});
Self { stop_thread: stop_thread_clone, thread_handle: handle }
Self { stop_thread: stop_thread_clone, thread_handle: Some(handle) }
}

/// Join `BackgroundProcessor`'s thread, returning any error that occurred while persisting
/// [`ChannelManager`].
///
/// # Panics
///
/// This function panics if the background thread has panicked such as while persisting or
/// handling events.
///
/// [`ChannelManager`]: lightning::ln::channelmanager::ChannelManager
pub fn join(mut self) -> Result<(), std::io::Error> {
assert!(self.thread_handle.is_some());
self.join_thread()
}

/// Stop `BackgroundProcessor`'s thread, returning any error that occurred while persisting
/// [`ChannelManager`].
///
/// # Panics
///
/// This function panics if the background thread has panicked such as while persisting or
/// handling events.
///
/// [`ChannelManager`]: lightning::ln::channelmanager::ChannelManager
pub fn stop(mut self) -> Result<(), std::io::Error> {
assert!(self.thread_handle.is_some());
Comment thread
valentinewallace marked this conversation as resolved.
self.stop_and_join_thread()
}

/// Stop `BackgroundProcessor`'s thread.
pub fn stop(self) -> Result<(), std::io::Error> {
fn stop_and_join_thread(&mut self) -> Result<(), std::io::Error> {
self.stop_thread.store(true, Ordering::Release);
self.thread_handle.join().unwrap()
self.join_thread()
}

fn join_thread(&mut self) -> Result<(), std::io::Error> {
match self.thread_handle.take() {
Some(handle) => handle.join().unwrap(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that if the background thread panics, join will return an error that will cause the unwrap to panic on this line. Might it'd be preferable to either return the panic as an error or at least document that this is the expected behavior?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added docs about panicking to join and stop.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does feel a bit strange that we have a function that returns a Result, but instead of ever returning an Err it always panics.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, so the behavior of std's join() is to return an error if the thread panics. Might it make more sense for our join method to mirror that behavior?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does feel a bit strange that we have a function that returns a Result, but instead of ever returning an Err it always panics.

It will return an Err if the persister returns an Err. The unwrap is to remove the error handling JoinHandle provides for panics, not for the Result returned by the thread's closure.

Hmm, so the behavior of std's join() is to return an error if the thread panics. Might it make more sense for our join method to mirror that behavior?

My intention was to mirror the same behavior as stop, which would already panic in this case. The reasoning for panicking was because the panic would likely have originated from user code. So users should either write code to not panic in the case of event handling (some discussion here) or to return an error in case of persisting. If the panic is coming from RL, my assumption was we'd want to propagate it.

I'm not necessarily opposed to making it return the wrapped result, but I think we'd want stop to have the same behavior. There may be a good argument based on how a user configures Rust to handle panics. I vaguely remember @TheBlueMatt discussing it with us but I'm a bit foggy on the details.

None => Ok(()),
}
}
}

impl Drop for BackgroundProcessor {
fn drop(&mut self) {
self.stop_and_join_thread().unwrap();
}
}

Expand DownExpand Up@@ -416,7 +458,13 @@ mod tests {
let persister = |_: &_| Err(std::io::Error::new(std::io::ErrorKind::Other, "test"));
let event_handler = |_| {};
let bg_processor = BackgroundProcessor::start(persister, event_handler, nodes[0].chain_monitor.clone(), nodes[0].node.clone(), nodes[0].peer_manager.clone(), nodes[0].logger.clone());
let _ = bg_processor.thread_handle.join().unwrap().expect_err("Errored persisting manager: test");
match bg_processor.join() {
Ok(_) => panic!("Expected error persisting manager"),
Err(e) => {
assert_eq!(e.kind(), std::io::ErrorKind::Other);
assert_eq!(e.get_ref().unwrap().to_string(), "test");
},
}
}

#[test]
Expand Down
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 64 additions & 16 deletions lightning-background-processor/src/lib.rs
Original file line numberDiff line numberDiff line change
Expand Up@@ -41,9 +41,7 @@ use std::ops::Deref;
/// for unilateral chain closure fees are at risk.
pub struct BackgroundProcessor {
stop_thread: Arc<AtomicBool>,
/// May be used to retrieve and handle the error if `BackgroundProcessor`'s thread
/// exits due to an error while persisting.
pub thread_handle: JoinHandle<Result<(), std::io::Error>>,
thread_handle: Option<JoinHandle<Result<(), std::io::Error>>>,
}

#[cfg(not(test))]
Expand DownExpand Up@@ -84,21 +82,25 @@ ChannelManagerPersister<Signer, M, T, K, F, L> for Fun where
}

impl BackgroundProcessor {
/// Start a background thread that takes care of responsibilities enumerated in the top-level
/// documentation.
/// Start a background thread that takes care of responsibilities enumerated in the [top-level
/// documentation].
///
/// If `persist_manager` returns an error, then this thread will return said error (and
/// `start()` will need to be called again to restart the `BackgroundProcessor`). Users should
/// wait on [`thread_handle`]'s `join()` method to be able to tell if and when an error is
/// returned, or implement `persist_manager` such that an error is never returned to the
/// `BackgroundProcessor`
/// The thread runs indefinitely unless the object is dropped, [`stop`] is called, or
/// `persist_manager` returns an error. In case of an error, the error is retrieved by calling
/// either [`join`] or [`stop`].
///
/// Typically, users should either implement [`ChannelManagerPersister`] to never return an
/// error or call [`join`] and handle any error that may arise. For the latter case, the
/// `BackgroundProcessor` must be restarted by calling `start` again after handling the error.
///
/// `persist_manager` is responsible for writing out the [`ChannelManager`] to disk, and/or
/// uploading to one or more backup services. See [`ChannelManager::write`] for writing out a
/// [`ChannelManager`]. See [`FilesystemPersister::persist_manager`] for Rust-Lightning's
/// provided implementation.
///
/// [`thread_handle`]: BackgroundProcessor::thread_handle
/// [top-level documentation]: Self
/// [`join`]: Self::join
/// [`stop`]: Self::stop
/// [`ChannelManager`]: lightning::ln::channelmanager::ChannelManager
/// [`ChannelManager::write`]: lightning::ln::channelmanager::ChannelManager#impl-Writeable
/// [`FilesystemPersister::persist_manager`]: lightning_persister::FilesystemPersister::persist_manager
Expand DownExpand Up@@ -158,13 +160,53 @@ impl BackgroundProcessor {
}
}
});
Self { stop_thread: stop_thread_clone, thread_handle: handle }
Self { stop_thread: stop_thread_clone, thread_handle: Some(handle) }
}

/// Join `BackgroundProcessor`'s thread, returning any error that occurred while persisting
/// [`ChannelManager`].
///
/// # Panics
///
/// This function panics if the background thread has panicked such as while persisting or
/// handling events.
///
/// [`ChannelManager`]: lightning::ln::channelmanager::ChannelManager
pub fn join(mut self) -> Result<(), std::io::Error> {
assert!(self.thread_handle.is_some());
self.join_thread()
}

/// Stop `BackgroundProcessor`'s thread, returning any error that occurred while persisting
/// [`ChannelManager`].
///
/// # Panics
///
/// This function panics if the background thread has panicked such as while persisting or
/// handling events.
///
/// [`ChannelManager`]: lightning::ln::channelmanager::ChannelManager
pub fn stop(mut self) -> Result<(), std::io::Error> {
assert!(self.thread_handle.is_some());
Comment thread
valentinewallace marked this conversation as resolved.
self.stop_and_join_thread()
}

/// Stop `BackgroundProcessor`'s thread.
pub fn stop(self) -> Result<(), std::io::Error> {
fn stop_and_join_thread(&mut self) -> Result<(), std::io::Error> {
self.stop_thread.store(true, Ordering::Release);
self.thread_handle.join().unwrap()
self.join_thread()
}

fn join_thread(&mut self) -> Result<(), std::io::Error> {
match self.thread_handle.take() {
Some(handle) => handle.join().unwrap(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that if the background thread panics, join will return an error that will cause the unwrap to panic on this line. Might it'd be preferable to either return the panic as an error or at least document that this is the expected behavior?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added docs about panicking to join and stop.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does feel a bit strange that we have a function that returns a Result, but instead of ever returning an Err it always panics.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, so the behavior of std's join() is to return an error if the thread panics. Might it make more sense for our join method to mirror that behavior?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does feel a bit strange that we have a function that returns a Result, but instead of ever returning an Err it always panics.

It will return an Err if the persister returns an Err. The unwrap is to remove the error handling JoinHandle provides for panics, not for the Result returned by the thread's closure.

Hmm, so the behavior of std's join() is to return an error if the thread panics. Might it make more sense for our join method to mirror that behavior?

My intention was to mirror the same behavior as stop, which would already panic in this case. The reasoning for panicking was because the panic would likely have originated from user code. So users should either write code to not panic in the case of event handling (some discussion here) or to return an error in case of persisting. If the panic is coming from RL, my assumption was we'd want to propagate it.

I'm not necessarily opposed to making it return the wrapped result, but I think we'd want stop to have the same behavior. There may be a good argument based on how a user configures Rust to handle panics. I vaguely remember @TheBlueMatt discussing it with us but I'm a bit foggy on the details.

None => Ok(()),
}
}
}

impl Drop for BackgroundProcessor {
fn drop(&mut self) {
self.stop_and_join_thread().unwrap();
}
}

Expand DownExpand Up@@ -416,7 +458,13 @@ mod tests {
let persister = |_: &_| Err(std::io::Error::new(std::io::ErrorKind::Other, "test"));
let event_handler = |_| {};
let bg_processor = BackgroundProcessor::start(persister, event_handler, nodes[0].chain_monitor.clone(), nodes[0].node.clone(), nodes[0].peer_manager.clone(), nodes[0].logger.clone());
let _ = bg_processor.thread_handle.join().unwrap().expect_err("Errored persisting manager: test");
match bg_processor.join() {
Ok(_) => panic!("Expected error persisting manager"),
Err(e) => {
assert_eq!(e.kind(), std::io::ErrorKind::Other);
assert_eq!(e.get_ref().unwrap().to_string(), "test");
},
}
}

#[test]
Expand Down
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 64 additions & 16 deletions lightning-background-processor/src/lib.rs
Original file line numberDiff line numberDiff line change
Expand Up@@ -41,9 +41,7 @@ use std::ops::Deref;
/// for unilateral chain closure fees are at risk.
pub struct BackgroundProcessor {
stop_thread: Arc<AtomicBool>,
/// May be used to retrieve and handle the error if `BackgroundProcessor`'s thread
/// exits due to an error while persisting.
pub thread_handle: JoinHandle<Result<(), std::io::Error>>,
thread_handle: Option<JoinHandle<Result<(), std::io::Error>>>,
}

#[cfg(not(test))]
Expand DownExpand Up@@ -84,21 +82,25 @@ ChannelManagerPersister<Signer, M, T, K, F, L> for Fun where
}

impl BackgroundProcessor {
/// Start a background thread that takes care of responsibilities enumerated in the top-level
/// documentation.
/// Start a background thread that takes care of responsibilities enumerated in the [top-level
/// documentation].
///
/// If `persist_manager` returns an error, then this thread will return said error (and
/// `start()` will need to be called again to restart the `BackgroundProcessor`). Users should
/// wait on [`thread_handle`]'s `join()` method to be able to tell if and when an error is
/// returned, or implement `persist_manager` such that an error is never returned to the
/// `BackgroundProcessor`
/// The thread runs indefinitely unless the object is dropped, [`stop`] is called, or
/// `persist_manager` returns an error. In case of an error, the error is retrieved by calling
/// either [`join`] or [`stop`].
///
/// Typically, users should either implement [`ChannelManagerPersister`] to never return an
/// error or call [`join`] and handle any error that may arise. For the latter case, the
/// `BackgroundProcessor` must be restarted by calling `start` again after handling the error.
///
/// `persist_manager` is responsible for writing out the [`ChannelManager`] to disk, and/or
/// uploading to one or more backup services. See [`ChannelManager::write`] for writing out a
/// [`ChannelManager`]. See [`FilesystemPersister::persist_manager`] for Rust-Lightning's
/// provided implementation.
///
/// [`thread_handle`]: BackgroundProcessor::thread_handle
/// [top-level documentation]: Self
/// [`join`]: Self::join
/// [`stop`]: Self::stop
/// [`ChannelManager`]: lightning::ln::channelmanager::ChannelManager
/// [`ChannelManager::write`]: lightning::ln::channelmanager::ChannelManager#impl-Writeable
/// [`FilesystemPersister::persist_manager`]: lightning_persister::FilesystemPersister::persist_manager
Expand DownExpand Up@@ -158,13 +160,53 @@ impl BackgroundProcessor {
}
}
});
Self { stop_thread: stop_thread_clone, thread_handle: handle }
Self { stop_thread: stop_thread_clone, thread_handle: Some(handle) }
}

/// Join `BackgroundProcessor`'s thread, returning any error that occurred while persisting
/// [`ChannelManager`].
///
/// # Panics
///
/// This function panics if the background thread has panicked such as while persisting or
/// handling events.
///
/// [`ChannelManager`]: lightning::ln::channelmanager::ChannelManager
pub fn join(mut self) -> Result<(), std::io::Error> {
assert!(self.thread_handle.is_some());
self.join_thread()
}

/// Stop `BackgroundProcessor`'s thread, returning any error that occurred while persisting
/// [`ChannelManager`].
///
/// # Panics
///
/// This function panics if the background thread has panicked such as while persisting or
/// handling events.
///
/// [`ChannelManager`]: lightning::ln::channelmanager::ChannelManager
pub fn stop(mut self) -> Result<(), std::io::Error> {
assert!(self.thread_handle.is_some());
Comment thread
valentinewallace marked this conversation as resolved.
self.stop_and_join_thread()
}

/// Stop `BackgroundProcessor`'s thread.
pub fn stop(self) -> Result<(), std::io::Error> {
fn stop_and_join_thread(&mut self) -> Result<(), std::io::Error> {
self.stop_thread.store(true, Ordering::Release);
self.thread_handle.join().unwrap()
self.join_thread()
}

fn join_thread(&mut self) -> Result<(), std::io::Error> {
match self.thread_handle.take() {
Some(handle) => handle.join().unwrap(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that if the background thread panics, join will return an error that will cause the unwrap to panic on this line. Might it'd be preferable to either return the panic as an error or at least document that this is the expected behavior?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added docs about panicking to join and stop.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does feel a bit strange that we have a function that returns a Result, but instead of ever returning an Err it always panics.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, so the behavior of std's join() is to return an error if the thread panics. Might it make more sense for our join method to mirror that behavior?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does feel a bit strange that we have a function that returns a Result, but instead of ever returning an Err it always panics.

It will return an Err if the persister returns an Err. The unwrap is to remove the error handling JoinHandle provides for panics, not for the Result returned by the thread's closure.

Hmm, so the behavior of std's join() is to return an error if the thread panics. Might it make more sense for our join method to mirror that behavior?

My intention was to mirror the same behavior as stop, which would already panic in this case. The reasoning for panicking was because the panic would likely have originated from user code. So users should either write code to not panic in the case of event handling (some discussion here) or to return an error in case of persisting. If the panic is coming from RL, my assumption was we'd want to propagate it.

I'm not necessarily opposed to making it return the wrapped result, but I think we'd want stop to have the same behavior. There may be a good argument based on how a user configures Rust to handle panics. I vaguely remember @TheBlueMatt discussing it with us but I'm a bit foggy on the details.

None => Ok(()),
}
}
}

impl Drop for BackgroundProcessor {
fn drop(&mut self) {
self.stop_and_join_thread().unwrap();
}
}

Expand DownExpand Up@@ -416,7 +458,13 @@ mod tests {
let persister = |_: &_| Err(std::io::Error::new(std::io::ErrorKind::Other, "test"));
let event_handler = |_| {};
let bg_processor = BackgroundProcessor::start(persister, event_handler, nodes[0].chain_monitor.clone(), nodes[0].node.clone(), nodes[0].peer_manager.clone(), nodes[0].logger.clone());
let _ = bg_processor.thread_handle.join().unwrap().expect_err("Errored persisting manager: test");
match bg_processor.join() {
Ok(_) => panic!("Expected error persisting manager"),
Err(e) => {
assert_eq!(e.kind(), std::io::ErrorKind::Other);
assert_eq!(e.get_ref().unwrap().to_string(), "test");
},
}
}

#[test]
Expand Down