Unified write data API#552
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #552 +/- ##
==========================================
+ Coverage 79.70% 79.79% +0.08%
==========================================
Files 28 28
Lines 11936 12079 +143
==========================================
+ Hits 9514 9638 +124
- Misses 2422 2441 +19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
c7b318c to
af32dd9
Compare
| struct aws_http1_chunk_options termination_opts; | ||
| AWS_ZERO_STRUCT(termination_opts); |
There was a problem hiding this comment.
this should invoke the callback.
In case of options->data != NULL && options->end_stream
I think more reasonable way is skip the on_complete for the first chunk with data, and then provide the callback to the termination chunk.
| struct aws_http_stream_write_data_options write_options = { | ||
| .data = NULL, | ||
| .end_stream = true, | ||
| }; |
There was a problem hiding this comment.
add a callback and make sure the callback invoked correctly
| return AWS_OP_SUCCESS; | ||
| } | ||
|
|
||
| /* Test: write_data with NULL data and end_stream=true on chunked stream */ |
There was a problem hiding this comment.
what about a case with data and end_stream = true for chunked encoding?
There was a problem hiding this comment.
is that not the write_chunk case (the happy path that used to be tested already)?
| stream->synced_data.is_cross_thread_work_task_scheduled = true; | ||
| } | ||
|
|
||
| stream->synced_data.has_final_data_write = options->end_stream; |
There was a problem hiding this comment.
i think we should check when end_stream set, the provided data is mathcing the content-length or not, when options->data is NULL.
Since you skip create data_write when there is no data, the corner case will not be checked. Also, there is no place to invoke the complete callback.
So, maybe another concern case worth to add a test for
16def17 to
944d1e5
Compare
| /* Whether the stream is using manual data writes instead of input_stream */ | ||
| bool using_manual_data_writes : 1; |
| } | ||
|
|
||
| if (encoder_message->content_length > 0 && !has_body_stream) { | ||
| if (use_manual_data_writes && has_body_stream) { |
There was a problem hiding this comment.
Actually added the check here.. forgot that I had done it.
Issue #, if available:
Description of changes:
A new protocol-agnostic aws_http_stream_write_data() API that lets users write request body data the same way regardless of whether the connection is HTTP/1.1 or HTTP/2.
High level details of changes:
Public API:
H1 Content-Length path:
H1 Chunked path:
H2 path:
Vtable unification:
Elasticurl example:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.