diff --git a/0 b/0 index 03a2c2ba0..ebaae1956 100644 --- a/0 +++ b/0 @@ -1 +1 @@ -{"blob":"1215c2a590644a5cf475a930c9544d6bd8b22844","description":"These are the keys of @fintohaps and @cloudhead's laptop key.","parent":"60ab40d9f4116c8ea5dcb81ac78c48fb69b5268e","signature":"z2YFFQ2xHTddV7EeWKymNDo8PbLCqpuGW3Q4vx3GMscHepcLedMKvfURyCkS7mCUt9ERe17mX79q15GtyezpMEDzB","title":"Add two more delegates","type":"revision"} \ No newline at end of file +{"body":"`radicle_signals::handler` should be async-signal-safe, as implied by\nthe POSIX [[1]] [[2]] and C (C17 7.14.1.1.5) requirements. (On Windows,\nwhere asynchronously-generated signals don't exist (IIUC), this isn't\nrelevant.)\n\nAt issue in `radicle_signals::handler` are its use of:\n- `std::sync::Mutex::try_lock`, and its `MutexGuard` dropping that does\n the separate unlocking operation.\n- `crossbeam_channel::Sender::try_send`.\n\n# Rationale\n\nNeither of those are specified as being async-signal-safe. While their\ncurrent internal implementations probably are on Linux (due to Linux\nfutexes being used internally for both), this should not be assumed,\nbecause it should be portable to other OSs where whatever is used\ninternally might not be async-signal-safe.\n\nRust's `std::sync::Mutex` makes no guarantees about what it uses\ninternally nor about whether it's async-signal-safe, and so its\nimplementation for some POSIX OSs might not be async-signal-safe.\nE.g. the `pthread_mutex_trylock` of Mac OS X is not async-signal-safe,\napparently [[3]], and POSIX does not consider any of the Pthreads mutex\nfunctions to be async-signal-safe, and I think it would not be\nunreasonable for an impl of `std::sync::Mutex` to be based on Pthreads\nor the like (even though it often isn't).\n\nSimilarly for `crossbeam_channel` (and most other types of channels from\nother crates), I don't think its `Sender` guarantees whether its\ninternal implementation for all OSs will always be async-signal-safe.\n\nEven on Linux, I don't think it should be assumed that higher-level\nAPIs, that are not included among those that are explicitly listed as\nasync-signal-safe, will always use futexes and only futexes.\n\nThe recommended practice is that the only functions a signal handler\nshould use (including transitively) are those that are explicitly listed\nas async-signal-safe. Unfortunately, the functionalities those offer\nare extremely limited (due to the nature of signal delivery interrupting\narbitrary threads during arbitrary functions at arbitrary instructions).\n\n(I happened to be already refreshed on signal handling, a few weeks ago,\nbefore I knew about Radicle, and then I wanted to do more research for\nthis issue.)\n\n# Objective\n\nThe primary challenge is how a blocked thread, that wants to consume\nnotifications of incoming signals, can be woken from within an\nextremely-limited signal handler.\n\nIt's not absolutely necessary to be able to send the notifications via a\nchannel to the other consuming thread, because atomic variables can be\nused instead from within a signal handler to record the receipt of\nsignals, and the other thread can access those to find out which signals\nwere received.\n\n# Options to change `radicle_signals::handler`:\n\n1. Use the [`signal_hook`][4] crate. Its [iterator][5] over incoming\n signals looks easy to initialize and use across threads [[6]], and I\n think this could work for the \"signals\" thread of `radicle_node`.\n This crate, and its underlying `signal_hook_registry` crate, also\n provide some other abilities that might be of use.\n\n I'll need to become more familiar with `radicle_term`, to evaluate\n whether `signal_hook` could work for its needs, including for its\n need to uninstall signal handling.\n\n `signal_hook` is entirely async-signal-safe and will remain so. It\n provides an impressive degree of abilities, for being so limited by\n async-signal-safety, and it goes to some lengths internally (it has\n its own unusual \"half-lock\" and channel types that are carefully\n designed to be async-signal-safe by only using atomics). It also\n does the best that can be done to deal with the race conditions that\n are possible when initializing signal handlers. It seems somewhat\n mature and has significant users (Tokio being a big one), so it seems\n reliable into the future. It's not a heavy dependency, but it's not\n the smallest either. Its full suite of abilities might be of use if\n `radicle_signals` needs to be able to do additional things in the\n future. I've done an initial reading of some of its source code and\n it seemed good to me so far.\n\n2. Use the classic \"self-pipe trick\" where `write()`, which is\n async-signal-safe, to a pipe is done from a signal handler, and where\n blocking `read()` from the other end of the pipe is done from the\n other consuming thread to wake it when a signal is received. (This\n is what `signal_hook::iterator` does.)\n\n But implementing this by one's self would require `radicle_signals`\n to invent its own way of using atomic types, or of encoding in the\n bytes sent in the pipe, to exfiltrate, from the signal handler, a\n representation of notification that the other thread would consume\n when woken. (This is what `signal_hook::iterator` does internally\n for its iterator.) Initializing the \"self-pipe trick\" is somewhat\n messier, due to needing to setup the pipes, close-on-exec, and\n non-blocking writes. (`signal_hook::iterator` sets this up properly\n for you.)\n\n3. Use the `sem_post` function of POSIX, which is guaranteed to be\n async-signal-safe and is specifically pointed-out by POSIX as being\n of use from within signal handlers. The other consuming thread would\n use the `sem_wait` function to block on and be woken from. This is\n simpler and cleaner than the \"self-pipe trick\". But, like that, this\n would require `radicle_signals` to invent its own way of exfiltrating\n signal notifications in a purely async-signal-safe way.\n\n I recently made a Rust-ified and safe interface to `sem_init`,\n `sem_post`, & `sem_wait` (et al) [[7]] that is simple enough that I\n think it could be trustworthy soon. I also started a brainstorm\n implementation, using my `sem_safe` crate, of signals-receipt\n exfiltration, where signal notifications are simple atomic counters,\n one for each signal type (signal number) of interest, and where the\n signal handler just atomically increments a signal's count, based on\n the received signal number, and does `sem_post()` to wake the other\n consuming thread, and where the other thread atomically checks the\n counters to see which need to be dealt with however desired. I had\n some ideas for trying to make this somewhat ergonomic. I worked on\n this for my own fun, since I thought I might want such for my own\n projects, and I'm totally fine if you're not interested in this.\n\n4. Unfortunately, the other classic approach that is often recommended -\n using `sigwait` (or `sigwaitinfo` or `sigtimedwait`) from the\n consuming thread to synchronously receive signals and to block that\n thread until they're delivered, while altogether avoiding signal\n handlers and so avoiding async-signal-safety hassles - would probably\n be too undesirable.\n\n This approach requires \"blocking\" (masking) all signals (except for\n those like `SIGSEGV` that correspond to a \"computational exception\"),\n i.e. using `pthread_sigmask` to change the signal mask at the start\n of `main` to be inherited by all threads. The issue is that the\n signal mask would also be inherited by all child processes by\n default, and, with the very many subprocesses that Radicle uses, that\n signal mask would have to be undone during the setup of each and\n every subprocess, after the `fork()` and before the `exec()` (which\n can be achieved via `std::os::unix::process::CommandExt::pre_exec`).\n\n But that seems too error-prone, because, while a helper for always\n doing this to each `Command` builder could be made, using it could be\n forgotten and that would significantly break the programs being run\n as subprocesses, because they'd have all signals blocked but they\n sometimes need to receive some signals and almost no programs are\n prepared to be started with all signals blocked. (If signal masks\n weren't inherited across `exec()`, then the `sigwait` approach might\n be ideal.)\n\nIf you like, I'm happy to work on any approach you want.\n\n# Related\n\nI'll be opening two further issues related to `radicle_signals`. One of\nthem is about a bug with using mutex'ing causing back-to-back signals to\nbe missed while the mutex is locked, and, depending on the fix chosen,\nthe fix for that might affect, improve, or resolve the\nasync-signal-safety as described here in this issue.\n\n[1]: https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_27\n[2]: https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04_03_03\n[3]: https://stackoverflow.com/questions/76074144/why-is-pthread-mutex-trylock-not-an-async-signal-safe-function\n[4]: https://crates.io/crates/signal-hook\n[5]: https://docs.rs/signal-hook/latest/signal_hook/iterator/index.html\n[6]: https://docs.rs/signal-hook/latest/signal_hook/iterator/struct.SignalsInfo.html#examples\n[7]: https://app.radicle.xyz/nodes/ash.radicle.garden/rad:zUAeNZN24JCzNQ9YVkoxNbw8c8TU","type":"comment"} \ No newline at end of file diff --git a/1 b/1 new file mode 100644 index 000000000..de39d3463 --- /dev/null +++ b/1 @@ -0,0 +1 @@ +{"title":"signals: `handler` is not async-signal-safe","type":"edit"} \ No newline at end of file diff --git a/embeds/radicle.json b/embeds/radicle.json deleted file mode 100644 index 1215c2a59..000000000 --- a/embeds/radicle.json +++ /dev/null @@ -1 +0,0 @@ -{"delegates":["did:key:z6MksFqXN3Yhqk8pTJdUGLwATkRfQvwZXPqR2qMEhbS9wzpT","did:key:z6MktaNvN1KVFMkSRAiN4qK5yvX1zuEEaseeX5sffhzPZRZW","did:key:z6MkireRatUThvd3qzfKht1S44wpm4FEWSSa4PRMTSQZ3voM"],"payload":{"xyz.radicle.project":{"defaultBranch":"master","description":"Radicle Heartwood Protocol & Stack","name":"heartwood"}},"threshold":1} \ No newline at end of file diff --git a/manifest b/manifest index e7c153005..6b7f0ba52 100644 --- a/manifest +++ b/manifest @@ -1 +1 @@ -{"typeName":"xyz.radicle.id","version":1} \ No newline at end of file +{"typeName":"xyz.radicle.issue","version":1} \ No newline at end of file