AndrewBarba / fetch-http2

Native http2 fetch implementation for Node.js
MIT License
5 stars 1 forks source link

Fixed race condition when send the request and subscribe to error/response events #2

Closed RodolfoGS closed 1 year ago

RodolfoGS commented 1 year ago

Hello,

I was checking the code before integrating it, and I noticed a possible race condition.

You're calling req.end() before subscribing to the request error / response events. So, in the case that the server responds before the listeners, the code will be locked forever.

I tested it by wrapping the content of _responseHeaders into a setTimeout to reproduce the case.

If you simply move req.end() after the req.on(... events are created, you could avoid this possible race condition and it is a good practice to avoid potential other future issues.

AndrewBarba commented 1 year ago

@RodolfoGS This is not the case as the event listens are added in the same event loop tick as calling req.end(). I/O in node will resume as soon as the current event loop is complete and before starting the next event loop tick. The change you have here is actually worse because we have an await on the response before ending the request. Going to close this and keep current behavior

AndrewBarba commented 1 year ago

I apologize I mis-read where you added the req.end()! I still think current code is fine but I like where you put it inside response headers, going to merge this

AndrewBarba commented 1 year ago

@RodolfoGS Published https://github.com/AndrewBarba/fetch-http2/releases/tag/1.2.1