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
