Sendable Response #3082
Sendable Response #3082
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3082 +/- ##
==========================================
- Coverage 76.34% 76.23% -0.11%
==========================================
Files 211 211
Lines 7862 7904 +42
==========================================
+ Hits 6002 6026 +24
- Misses 1860 1878 +18
|
| } | ||
| set { | ||
| _body.withLockedValue { $0 = newValue } | ||
| self.headers.updateContentLength(self.body.count) |
There was a problem hiding this comment.
perhaps we could just do self.headers.updateContentLength(newValue) and skip a _body.withLockedValue { $0 }?
|
There are some warnings in the CI which don't seem blocking for this PR:
API breakage also seems to be inaccurate per usual. |
| private let _version: NIOLockedValueBox<HTTPVersion> | ||
| private let _status: NIOLockedValueBox<HTTPResponseStatus> | ||
| private let _headers: NIOLockedValueBox<HTTPHeaders> | ||
| private let _body: NIOLockedValueBox<Body> | ||
| private let _upgrader: NIOLockedValueBox<Upgrader?> | ||
| private let _storage: NIOLockedValueBox<Storage> |
There was a problem hiding this comment.
Fine-grained locking is all well and good for efficiency, but this is an awful lot of additional allocations on a per-response basis. You can easily combine, at the very least, version, status, and headers under a single lock - it's exceedingly rare (in fact, it might never happen at all) to need to access any of these values simultaneously with one or both of the others from another thread, so contention ought not to be an issue. It might even be reasonable to include body in the same lock as well, though I'm less certain on that one.
(Truthfully, I might wonder if it didn't make more sense to put everything under a single lock, maybe with the exception of storage.)
There was a problem hiding this comment.
I mean, for example, look at the implementation of description and think for a second about just how painful that's going to actually be at runtime...
There was a problem hiding this comment.
@0xTim I can't see a new commit on this branch 🤔
There was a problem hiding this comment.
I mean it would really help if I actually pushed my changes
Co-authored-by: Mahdi Bahrami <github@mahdibm.com>
|
These changes are now available in 4.85.0 |
Make
ResponseSendable