NIOThread refactor: pthread_t lifetimes #3297
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
weissi
force-pushed
the
jw-niothread-refactor
branch
3 times, most recently
from
21622e1 to
50c018f
Compare
Lukasa
reviewed
Jul 11, 2025
| } | ||
| """ | ||
| } else { | ||
| return self.externalStateLock.withLock { |
There was a problem hiding this comment.
Why is the lock-holding concern not as relevant here?
There was a problem hiding this comment.
- it already holds a lock, so I'm not making things worse I think :P
- it's
debugDescriptionso maybe less likely to be called?
There was a problem hiding this comment.
Basically replicating prior art in the existing impl
| @@ -42,41 +44,74 @@ protocol ThreadOps { | |||
| /// All methods exposed are thread-safe. | |||
| @usableFromInline | |||
| final class NIOThread: Sendable { | |||
| internal typealias ThreadBoxValue = (body: (NIOThread) -> Void, name: String?) | |||
| internal typealias ThreadBoxValue = (body: (NIOThread) -> Void, name: String?, isDetached: Bool) | |||
There was a problem hiding this comment.
Should we tweak this API to try to discourage creating detached threads outside of tests?
There was a problem hiding this comment.
@Lukasa Honestly, I'm planning a follow-up to remove the ability to use detached threads at all, we don't use them outside of tests and we don't need them in tests.
Merged
weissi
added a commit
that referenced
this pull request
Jul 14, 2025
### Motivation: PR #3297 is changing `inEventLoop` and `inEventLoop` is very much on the perf path. So let's add a benchmark ### Modifications: - Add `(!)inEventLoop` benchmark - Fix a guaranteed deadlock in `TCPThroughputBenchmark` to make the benchmarks runnable ### Result: - More benchmarks - Fewer hangs
weissi
added a commit
that referenced
this pull request
Jul 14, 2025
### Motivation: `inEventLoop` is very much in the performance path of SwiftNIO, especially these days with Concurrency, `NIOLoopBound` and friends. Previously, we relied on `pthread_equal(pthread_self(), myPthread)`, however, this could cause a number of issues. 1. Holding onto a `pthread_t` after `.join` is actually illegal (fixed in #3297) 2. Potential ABA issues when `pthread_t` pointer are re-used for new pthreads 3. Fix would require a lock around `myPthread` which makes things (2x slower, even without contention) ### Modifications: - New type `SelectableEventLoopUniqueID` which can be packed into a `UInt` - Attach them into a C thread local ### Result: - Even faster than the old, incorrect version - old: `measuring: el_in_eventloop_100M: 0.257395375, 0.241049208, 0.243188792, 0.259125916, 0.24843225, 0.229690125, 0.244281541, 0.225078834, 0.236395, 0.233305167` - new: `measuring: el_in_eventloop_100M: 0.175561125, 0.187225625, 0.199269375, 0.19740975, 0.1922695, 0.179850958, 0.177612458, 0.17665125, 0.17897475, 0.18038775` - More correct - Groundwork to make #3297 not make things slower
weissi
force-pushed
the
jw-niothread-refactor
branch
2 times, most recently
from
112eb96 to
d22b9c5
Compare
weissi
force-pushed
the
jw-niothread-refactor
branch
from
d22b9c5 to
98ab4ea
Compare
Lukasa
approved these changes
Jul 14, 2025
weissi
force-pushed
the
jw-niothread-refactor
branch
4 times, most recently
from
f9e0e30 to
78dc04a
Compare
weissi
force-pushed
the
jw-niothread-refactor
branch
from
78dc04a to
5c4f499
Compare
zaneenders
pushed a commit
to zaneenders/swift-nio
that referenced
this pull request
Jul 23, 2025
### Motivation: `inEventLoop` is very much in the performance path of SwiftNIO, especially these days with Concurrency, `NIOLoopBound` and friends. Previously, we relied on `pthread_equal(pthread_self(), myPthread)`, however, this could cause a number of issues. 1. Holding onto a `pthread_t` after `.join` is actually illegal (fixed in apple#3297) 2. Potential ABA issues when `pthread_t` pointer are re-used for new pthreads 3. Fix would require a lock around `myPthread` which makes things (2x slower, even without contention) ### Modifications: - New type `SelectableEventLoopUniqueID` which can be packed into a `UInt` - Attach them into a C thread local ### Result: - Even faster than the old, incorrect version - old: `measuring: el_in_eventloop_100M: 0.257395375, 0.241049208, 0.243188792, 0.259125916, 0.24843225, 0.229690125, 0.244281541, 0.225078834, 0.236395, 0.233305167` - new: `measuring: el_in_eventloop_100M: 0.175561125, 0.187225625, 0.199269375, 0.19740975, 0.1922695, 0.179850958, 0.177612458, 0.17665125, 0.17897475, 0.18038775` - More correct - Groundwork to make apple#3297 not make things slower
zaneenders
pushed a commit
to zaneenders/swift-nio
that referenced
this pull request
Jul 23, 2025
### Motivation: NIOThread assumed it can hold onto `pthread_t` forever. Unfortunately, on Linux, `pthread_t` is dead when the thread got joined (if not detached) or if it exits (if detached) _which is at a random point in time_. ### Modifications: - Only assume ownership owner the `pthread_t` if not detached - `nil` out the `pthread_t` on `join()` ### Result: - Fewer `pthread_t` lifecycle bugs.
zaneenders
pushed a commit
to zaneenders/swift-nio
that referenced
this pull request
Jul 23, 2025
### Motivation: Structured Concurrency is helpful but so far, MTELG was very difficult to use with it mostly because Swift is still missing `async` in `defer`s. This builds on - apple#3297 - apple#3302 - apple#3304 ### Modifications: - Add MTELG.withELG { ... } ### Result: MTELG + SC = <3
Lukasa
pushed a commit
that referenced
this pull request
Aug 28, 2025
…3360) ### Motivation: In #3297, I introduced a deadlock involving `SelectableEventLoop`'s `debugDescription`. I was under the impression that it's not possible to call this from outside the `NIO` module so I deemed it safe. That was a mistake :). ### Modifications: - Make it impossible to deadlock around `SelectableEventLoop.debugDescription`. ### Result: - Fewer deadlocks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
NIOThread assumed it can hold onto
pthread_tforever. Unfortunately, on Linux,pthread_tis dead when the thread got joined (if not detached) or if it exits (if detached) which is at a random point in time.Modifications:
pthread_tif not detachednilout thepthread_tonjoin()Result:
pthread_tlifecycle bugs.