Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix hanging on stream response errors #2599

Merged
merged 1 commit into from
Jan 7, 2024

Conversation

kyri-petrou
Copy link
Collaborator

@kyri-petrou kyri-petrou commented Jan 7, 2024

/claim #2584

The reported behaviour was due to the channel not being closed when the stream errored. There are 2 ways that we can recover from such issues without causing clients to hang:

  1. Handle the error, potentially logging it (not added to this PR but happy to add it) and closing the netty channel
  2. Don't handle the error and let netty deal with it. Downside to this is that it causes netty to log a lot of errors which can get very noisy

With both approaches, what the client receives is the same response. Note that we cannot change the initial 200 OK response that was sent to the client.

curl -v http://127.0.0.1:8080/stream

*   Trying 127.0.0.1:8080...
* Connected to 127.0.0.1 (127.0.0.1) port 8080
> GET /stream HTTP/1.1
> Host: 127.0.0.1:8080
> User-Agent: curl/8.4.0
> Accept: */*
> 
< HTTP/1.1 200 OK
< transfer-encoding: chunked
< 
* transfer closed with outstanding read data remaining
* Closing connection
curl: (18) transfer closed with outstanding read data remaining

Alternative approach (requires user-facing breaking changes)

We could potentially change the Body.fromStream / Body.fromStreamX methods to force users to handle streaming errors (i.e., ZStream[R, Nothing, A]) and in cases of defects, leave it up to Netty to handle them. This would very likely require changes to users' code, but this option is less likely to result to unexpected behaviour due to ZStream failures

@codecov-commenter
Copy link

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (f4b2b3f) 64.23% compared to head (024fc13) 64.28%.

Files Patch % Lines
...a/zio/http/netty/server/ServerInboundHandler.scala 75.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2599      +/-   ##
==========================================
+ Coverage   64.23%   64.28%   +0.05%     
==========================================
  Files         140      140              
  Lines        8358     8359       +1     
  Branches     1604     1607       +3     
==========================================
+ Hits         5369     5374       +5     
+ Misses       2989     2985       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jdegoes
Copy link
Member

jdegoes commented Jan 7, 2024

The change to body to prevent users from using typed errors may be a good one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants