refactor(ext/http): Use HttpRecord as response body to track until body completion #20822
Conversation
|
It's worth taking a close look at 6548cc8. I'm not sure if we should delay Potentially there could be a data race which could lead to a use after free between the recycle and the op_http_track promise handler running. However I'm not sure this is actually possible given it is single threaded. To avoid this possibility we would add an op call in the promise handler where Rust would explicitly take ownership of the external and delay the EDIT: Updated with this approach which avoids the possibility of data race / use after free. |
lrowe
force-pushed
the
lrowe-http-record-response
branch
from
6548cc8
to
5ff4e6e
Compare
|
Worth checking if |
lrowe
force-pushed
the
lrowe-http-record-response
branch
from
5ff4e6e
to
a1ced4b
Compare
It's not, feel free to remove it if it that makes things easier. |
lrowe
force-pushed
the
lrowe-http-record-response
branch
3 times, most recently
from
cec80c8
to
3d0271a
Compare
|
I think this is ready for review. I removed |
|
/bench http[minimal,realistic] |
http[minimal]
http[realistic]
start: id: server: |
|
Blocked on denoland/deno_core#301 which will give us safer externals |
mmastrac
force-pushed
the
lrowe-http-record-response
branch
from
2f90970
to
356553c
Compare
|
/bench http[minimal,realistic] |
http[minimal]
http[realistic]
start: id: server: |
mmastrac
force-pushed
the
lrowe-http-record-response
branch
from
356553c
to
fbc3cc8
Compare
Now that requests are tracked until their body completes we no longer need to async spin on the server_state refcount but can check in HttpRecord::recycle.
mmastrac
force-pushed
the
lrowe-http-record-response
branch
from
fbc3cc8
to
c7132da
Compare
…dy completion (#20822) Use HttpRecord as response body so requests can be tracked all the way to response body completion. This allows Request properties to be accessed while the response body is streaming. Graceful shutdown now awaits a future instead of async spinning waiting for requests to finish. On the minimal benchmark this refactor improves performance an additional 2% over pooling alone for a net 3% increase over the previous deno main branch. Builds upon #20809 and #20770. --------- Co-authored-by: Matt Mastracci <[email protected]>
…dy completion (denoland#20822) Use HttpRecord as response body so requests can be tracked all the way to response body completion. This allows Request properties to be accessed while the response body is streaming. Graceful shutdown now awaits a future instead of async spinning waiting for requests to finish. On the minimal benchmark this refactor improves performance an additional 2% over pooling alone for a net 3% increase over the previous deno main branch. Builds upon denoland#20809 and denoland#20770. --------- Co-authored-by: Matt Mastracci <[email protected]>
Follow-up to #20822. cc @lrowe The `httpServerExplicitResourceManagement` tests were randomly failing on CI because of a race. The `drain` waker was missing wakeup events if the listeners shut down after the last HTTP response finished. If we lost the race (rare), the server Rc would be dropped and we wouldn't poll it again. This replaces the drain waker system with a signalling Rc that always resolves when the refcount is about to become 1. Fix verified by running serve tests in a loop: ``` for i in {0..100}; do cargo run --features=__http_tracing -- test -A --unstable '/Users/matt/Documents/github/deno/deno/cli/tests/unit/ser ve_test.ts' --filter httpServerExplicitResourceManagement; done; ```
Follow-up to #20822. cc @lrowe The `httpServerExplicitResourceManagement` tests were randomly failing on CI because of a race. The `drain` waker was missing wakeup events if the listeners shut down after the last HTTP response finished. If we lost the race (rare), the server Rc would be dropped and we wouldn't poll it again. This replaces the drain waker system with a signalling Rc that always resolves when the refcount is about to become 1. Fix verified by running serve tests in a loop: ``` for i in {0..100}; do cargo run --features=__http_tracing -- test -A --unstable '/Users/matt/Documents/github/deno/deno/cli/tests/unit/ser ve_test.ts' --filter httpServerExplicitResourceManagement; done; ``` (cherry picked from commit 68a0877)
Follow-up to #20822. cc @lrowe The `httpServerExplicitResourceManagement` tests were randomly failing on CI because of a race. The `drain` waker was missing wakeup events if the listeners shut down after the last HTTP response finished. If we lost the race (rare), the server Rc would be dropped and we wouldn't poll it again. This replaces the drain waker system with a signalling Rc that always resolves when the refcount is about to become 1. Fix verified by running serve tests in a loop: ``` for i in {0..100}; do cargo run --features=__http_tracing -- test -A --unstable '/Users/matt/Documents/github/deno/deno/cli/tests/unit/ser ve_test.ts' --filter httpServerExplicitResourceManagement; done; ```
Use HttpRecord as response body so requests can be tracked all the way to response body completion.
This allows Request properties to be accessed while the response body is streaming.
Graceful shutdown now awaits a future instead of async spinning waiting for requests to finish.
On the minimal benchmark this refactor improves performance an additional 2% over pooling alone for a net 3% increase over the previous deno main branch.
Builds upon #20809 and #20770.