feat(fetch) AbortSignal #2019
feat(fetch) AbortSignal #2019
Conversation
cirospaciari
commented
Feb 9, 2023
•
cirospaciari
commented
- onData abortion
- onWritable abortion
- addEventListener
- fail with the right reason
- detect TimeoutError and AbortError
- fix socket timeout to use TimeoutError
- add tests
src/bun.js/webcore/response.zig
Outdated
| @@ -688,6 +688,9 @@ pub const Fetch = struct { | |||
| } | |||
|
|
|||
| pub fn onReject(this: *FetchTasklet) JSValue { | |||
| if (this.result.exception) |exception| { | |||
There was a problem hiding this comment.
Does the memory still get freed?
There was a problem hiding this comment.
The code is pretty similar to toErrorInstance from SystemError, I moved all logic to onReject (the default fail SystemError is created here to using toErrorInstance) so I think we don't have any memory leaks here
src/http_client_async.zig
Outdated
| **anyopaque, | ||
| NewHTTPContext(is_ssl).ActiveSocket.init(&dead_socket).ptr(), | ||
| ); | ||
| if (this.globalThis) |globalThis| { |
There was a problem hiding this comment.
I don't think it's threadsafe to create JSValue here or for it to be aware of globalThis
I think it will need to enqueue a task to the event loop and to do this on the main thread
There was a problem hiding this comment.
I moved this to onReject which is called using task.javascript_vm.eventLoop().enqueueTaskConcurrent so will be threadsafe, and also I removed any reference to globalThis from http_client_async
src/http_client_async.zig
Outdated
| @@ -2147,6 +2255,7 @@ pub const HTTPClientResult = struct { | |||
| fail: anyerror = error.NoError, | |||
| redirected: bool = false, | |||
| headers_buf: []picohttp.Header = &.{}, | |||
| exception: ?JSC.JSValue = null, | |||
There was a problem hiding this comment.
I think the exception value itself will have to be part of the code that dispatches fetch instead of living here
There was a problem hiding this comment.
I changed this, the reason is already the exception I still pass using result because I need it in FetchTasklet
| beforeAll(() => { | ||
| server = Bun.serve({ | ||
| async fetch(){ | ||
| await Bun.sleep(2000); |
There was a problem hiding this comment.
There's a lot more we should test here
- what happens when you use ReadableStream and it's aborted?
- does request.signal.addListener work?
- what happens when you call request.signal.abort() but the request has already been responded to (does it crash?)
- what happens when you use ReadableStream type: "direct" and it's aborted?
- what happens when you use ReadableStream, you call stream.cancel() and then request.signal? Does it crash?
Later we should make Bun.spawn work with AbortSignal as well
There was a problem hiding this comment.
Working in the Request object signal and next I can make Bun.spawn, I will write those tests ASAP, you are right my tests are not enough, we need a more complete test suit.
Request object will affect fetch and maybe Bun.server (Force close connection)?
