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 HTTP/3 bug: uploading payload larger than 65536 will be blocked until timeout. #7538

Closed

Conversation

@MegatronKing
Copy link

@MegatronKing MegatronKing commented Aug 6, 2021

The upload buffer default size is 65536, if we post a payload larger than this, the first buffer is ok, but when sending the second buffer, the stream will be blocked with a curlcode CURLE_AGAIN. I supoose the cause is the upload_len was not reset.

lib/vquic/ngtcp2.c Outdated Show resolved Hide resolved
@bagder bagder added the HTTP/3 label Aug 6, 2021
lib/vquic/ngtcp2.c Outdated Show resolved Hide resolved
@bagder
Copy link
Member

@bagder bagder commented Aug 10, 2021

and the other comment, what do you think about it?

@MegatronKing
Copy link
Author

@MegatronKing MegatronKing commented Aug 10, 2021

@bagder
Actually, I am not very confident about my fix, I tested with all my POST cases and all approved.
I am confusing about the function 'cb_h3_readfunction', when and how it would be invoked? And this line

if(data->set.postfields) {
also is specific for POST and I am not able to understand too.

@MegatronKing
Copy link
Author

@MegatronKing MegatronKing commented Aug 10, 2021

My test cases:

  • POST application/octem 10/100/1024/1024 * 1024/10 * 1024 * 1024
  • POST multipart/form-data 10/100/1024/1024 * 1024/10 * 1024 * 1024
    And something intersting, POST multipart/form-data and data->set.postfields is false.

@bagder
Copy link
Member

@bagder bagder commented Aug 10, 2021

POST multipart/form-data and data->set.postfields is false

That's why I didn't like the fix. set.postfields is only set with CURLOPT_POSTFIELDS while you can send data over HTTP several other ways with libcurl. One of those ways you see. You can also try curl -T [file] -X POST $URL ...

So if you make the change I proposed, does it still work or isn't that good enough?

@MegatronKing
Copy link
Author

@MegatronKing MegatronKing commented Aug 10, 2021

if data->set.postfields is true, a flag NGHTTP3_DATA_FLAG_EOF is set and return, and the value of stream->upload_len would never be updated, right? I think this is a bug.

@MegatronKing MegatronKing force-pushed the fix/ngtcp2_stream_upload_bug branch from 0cbeb4f to 29e8844 Aug 10, 2021
@MegatronKing
Copy link
Author

@MegatronKing MegatronKing commented Aug 10, 2021

Agree with the fix is not good enough, I updated!

bagder
bagder approved these changes Aug 10, 2021
@bagder bagder linked an issue that may be closed by this pull request Aug 10, 2021
@bagder
Copy link
Member

@bagder bagder commented Aug 10, 2021

do you have a HTTP/3 test server somewhere that you can do large posts to, to verify this fix?

@bagder bagder closed this in 09cea3f Aug 10, 2021
@bagder
Copy link
Member

@bagder bagder commented Aug 10, 2021

Thanks, I pushed this as I believe it does improve things. I still experience problems if I upload multiple megabytes, but after all the data has been sent...

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

Successfully merging this pull request may close these issues.

None yet

2 participants