fastify / fastify-reply-from

fastify plugin to forward the current http request to another server
MIT License
148 stars 76 forks source link

fix: DELETE requests should not call stream.end #362

Closed karl-power closed 4 months ago

karl-power commented 4 months ago

It appears that DELETE streams behave the same as GET streams in that they are not writable (immediately ended?), so calling .end on the stream throws a write after end error (related issue).

I cannot find this documented anywhere in HTTP2 spec or in NodeJS HTTP2 module (aside from the endStream header, and http2stream.endAfterHeaders which don't seem to affect anything here - can anyone share insight?).

This PR simply adds the DELETE method to a check on whether or not to end the stream. The change fixes https://github.com/fastify/fastify-http-proxy/issues/253.

Checklist

gurgunday commented 4 months ago

It appears that DELETE streams behave the same as GET streams in that they are not writable (immediately ended?), so calling .end on the stream throws a write after end error (https://github.com/fastify/fastify-http-proxy/issues/253).

Yeah I thought for the same reasons when approving, that DELETE requests are like GET requests, but I just learned that they actually might have a body, so they can be writable (?) – maybe for all practical purposes it is interpreted the same way

Not 100% sure if this is the reason or not

karl-power commented 4 months ago

Hi @mcollina & @climba03003 Any idea if this is the right approach here?