report the error back to write complete #512
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #512 +/- ##
==========================================
- Coverage 79.49% 79.48% -0.01%
==========================================
Files 27 27
Lines 11684 11686 +2
==========================================
+ Hits 9288 9289 +1
- Misses 2396 2397 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
include/aws/http/private/h2_frames.h
Outdated
| @@ -223,7 +223,8 @@ int aws_h2_encode_data_frame( | |||
| size_t *connection_window_size_peer, | |||
| struct aws_byte_buf *output, | |||
| bool *body_complete, | |||
| bool *body_stalled); | |||
| bool *body_stalled, | |||
| bool *body_error); | |||
There was a problem hiding this comment.
utterly trivial: error isn't this most boolean-ish word
| bool *body_error); | |
| bool *body_failed); |
source/h2_stream.c
Outdated
| pending_write->error_code = AWS_ERROR_HTTP_MANUAL_WRITE_HAS_COMPLETED; | ||
| s_stream_data_write_destroy(stream, pending_write); |
There was a problem hiding this comment.
debatable/trivial: it seems a bit fragile that we be so careful about setting write->error_code before calling destroy(). It would be easy to miss a spot, and have a write complete with error_code 0 in some weird error situation.
Maybe go back to passing error_code to the destroy() function.
If a data_frame fails due a write, then destroy the write immediately, instead of leaving it in a list to get cleaned up later. That's how it works with HTTP/1 chunked encoding (see code). If failure was caused by the chunk, the chunk is immediately destroyed with that error code. Then connection shutdown continues...
source/h2_stream.c
Outdated
| if (input_stream_error) { | ||
| /* If the error comes from the input stream, propagate it to the current_write */ | ||
| current_write->error_code = aws_last_error(); |
There was a problem hiding this comment.
(continuing previous comment)
so like here, we could pop the current_write out of its list and destroy it
Issue #, if available:
Description of changes:
AWS_ERROR_HTTP_STREAM_HAS_COMPLETEDreported, the error SHOULD not be related to the data write.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.