Re: Should CURLOPT_READFUNCTION need EOF twice?

From: Jiří Hruška via curl-library <curl-library_at_lists.haxx.se>
Date: Mon, 27 Nov 2023 10:38:59 +0100

A week ago, Daniel Stenberg wrote:
> Here it is => https://github.com/curl/curl/pull/12363

Sorry for not being available for a while, thanks for taking care of it.
Good point about mixing EOF from data and trailers there too.

Let me just close this with a bit more info that I still gathered earlier.
I was trying to reproduce and test this more while preparing a PR and I
remembered that duh, it happened on HTTP/2, which has a protocol-level flow
control, that's probably how it got triggered in the first place. So I should
be able to simulate it after all, passing --frontend-http2-window-size=2000
or similar to nghttpx and that should make it work.

So I wrote a nice new libtest, CURLOPT_UPLOAD, CURLOPT_READFUNCTION, validate
that it gets called just once, add the --frontend-http2-window-size=2000 to
http2-server.pl, add some 100-continue hack to make sure the client actually
sees the small window before blasting out the default 64k too quickly, etc.

Aaaaand it did not reproduce. I verified that the data was sent in DATA blocks
of 2000 bytes over the HTTP/2 connection as expected and my test had to expect
them to come in these chunks on the server side, but Curl_fillreadbuffer() was
not called for them individually. It got called once with 65536 bytes of space
(UPLOADBUFFER_DEFAULT) and then again only after all of that was used.

I investigated why, and it turned out that this behavior changed in
"HTTP/2: upload handling fixes" (65937f0d, 2023-06-20, released in 8.2.0)
https://github.com/curl/curl/commit/65937f0d638f152d955e327d0fdaf14d7c98cb29

Before that, the HTTP/2 connection filter cf_h2_send() would work like e.g.
fwrite() - if only some of the data got accepted, it would "eat" a smaller
number of bytes between 1 and N and the caller could shift/refill the buffer.
This commit changed it to return 0 with CURLE_AGAIN instead, keeping the whole
buffer as it is, until everything gets processed.

So, the test I wrote does what it should when cherry-picked on top of that~1,
but no longer reproduces the issue after. Which probably means that:

1. My original problem would not reproduce since cURL 8.2.0 (Git4Win 2.42) too
   (and I should just upgrade stuff everywhere more often, and I should take
   less than 1 year to investigate and report bugs, yeah, fair enough).

2. The whole code with `ssize_t offset` and partial upload buffer refilling
   in readwrite_upload() seems pointless now, because afaics, it has been
   all added only specifically for HTTP/2 and nghttp2 in this commit:
   https://github.com/curl/curl/commit/7f43f3dc5994d01b12eb1dfe2ebf5e8371b4e32b
   But now the HTTP/2 path never partially uses a buffer, and so perhaps
   this whole extra complexity there along with curl_upload_refill_watermark()
   magic numbers etc. could be removed again. And as said before, no hits of
   offset != 0 ever in the whole test suite either (even when executed with
   nghttpx --frontend-http2-window-size=2000).

3. As the bug could realistically reproduce only between 7.84.0 and 8.1.2,
   there does not seem to be a strong reason to bother anymore, and also
   if no reports of CURLOPT_READFUNCTION called twice were ever received
   outside of these versions and HTTP/2, my whole point here is moot and
   probably nothing needed to be done at all...

So, in summary, sorry for the trouble, and I'll do a better job next time :-)

Kind regards,
Jiri

-- 
Unsubscribe: https://lists.haxx.se/mailman/listinfo/curl-library
Etiquette:   https://curl.se/mail/etiquette.html

Received on 2023-11-27