Avoid writing to the body after full hijack

yard commented 3 months ago

Whenever the application performs hijacking of http/1.1 connection, the server must not attempt to send any response back to the client. Prior to this change, it was not the case due to imprecise check and has led to an error being logged (the real issue occurred during an attempt to write to a stream that was set to nil, but that's really just a symptom).

The proposed fix simply adds a flag to the connection object so that it is aware of the fact the underlying stream no longer belongs to it and must not be used to send any data over.

Types of Changes

Bug fix.

Contribution

[x] I added tests for my changes.
[x] I tested my changes locally.
[x] I agree to the Developer's Certificate of Origin 1.1.

ioquatix commented 3 months ago

This looks okay to me, but my only thought is whether we should move this to protocol-http1.

yard commented 2 months ago

Actually a good point, maybe we should (since #hijack! is part of protocol-http1). This will, however, require bumping the dependency version here too.

Do lmk if you want me to split this PR into 2 – happy to do that.

ioquatix commented 2 months ago

Yes, let's implement the core functionality in protocol-http1 if possible - thanks!!

yard commented 2 months ago

As soon as https://github.com/socketry/protocol-http1/pull/30 lands I can update this PR – will only need protocol-http1 to be released so that I can bump the version in this gem.

ioquatix commented 2 months ago

protocol-http1 v0.19.0 was released.

yard commented 2 months ago

Updated this PR to match

ioquatix commented 2 months ago

I tried this out and it's working as expected - thanks!

socketry / async-http

Avoid writing to the body after full hijack #151

Types of Changes

Contribution