feat(Request.signal) Initial support for signal in Request + fetch and Request + Bun.serve #2097
Conversation
… creation to main thread
test/bun.js/fetch.test.js
Outdated
| "http://localhost:64321/stream", | ||
| { | ||
| method: "POST", | ||
| body: new ReadableStream({ |
There was a problem hiding this comment.
start is executed synchronously, so this test does nothing
|
I don't think we can ship this until it also works with Bun.serve() because people will expect it to work there too |
added |
cirospaciari
changed the title
| void AbortSignal::cleanNativeBindings(void* ref) { | ||
| auto callbacks = std::exchange(m_native_callbacks, {}); | ||
|
|
||
| callbacks.removeLastMatching([=](auto callback){ |
There was a problem hiding this comment.
should it be removeLastMatching or remove all matching?
There was a problem hiding this comment.
removeAllMatching is the right answer, fixed on 483a9f8
src/bun.js/api/server.zig
Outdated
| if (request_js.as(Request)) |req| { | ||
| if (req.signal) |signal| { | ||
| // if signal is not aborted, abort the signal | ||
| if(!signal.aborted()){ |
There was a problem hiding this comment.
There are two places we need this code in, grep for the comment above
|
can you add a test to the list of ReactDOM tests which sees how that handles aborting in the middle of a ReadableStream? after that I think one or 2 more tests with ReadableStream |
| req.signal.addEventListener("abort", () => { | ||
| signalOnServer = true; | ||
| }); | ||
| await Bun.sleep(3000); |
There was a problem hiding this comment.
can we make these timeouts shorter so the test doesn't take as long, but still assert sequential order?
There was a problem hiding this comment.
after that i think its good to merge
No description provided.