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

http: set content length earlier #7803

Closed
wants to merge 1 commit into from

Conversation

jay
Copy link
Member

@jay jay commented Oct 1, 2021

  • Make content length (ie download size) accessible to the user in the
    header callback, but only after all headers have been processed (ie
    only in the final call to the header callback).

Background:

For a long time the content length could be retrieved in the header
callback via CURLINFO_CONTENT_LENGTH_DOWNLOAD_T as soon as it was parsed
by curl.

Changes were made in 8a16e54 (precedes 7.79.0) to ignore content length
if any transfer encoding is used. A side effect of that was that
content length was not set by libcurl until after the header callback
was called the final time, because until all headers are processed it
cannot be determined if content length is valid.

This change keeps the same intention --all headers must be processed--
but now the content length is available before the final call to the
header function that indicates all headers have been processed (ie
a blank header).

Bug: 8a16e54#r57374914
Reported-by: sergio-nsk@users.noreply.github.com

Fixes #7804
Closes #xxxx

@jay jay added the HTTP label Oct 1, 2021
@jay jay force-pushed the make_contentlen_avail_in_headercb branch 2 times, most recently from 8e819f9 to d1a3e96 Compare October 1, 2021 18:51
@jay
Copy link
Member Author

jay commented Oct 1, 2021

changed it to deduplicate a separate similar check that occurs after all headers have been received

@jay
Copy link
Member Author

jay commented Oct 1, 2021

some hyper tests for maximum filesize are failing so I have to look into that. the other CI failures don't make sense, they are all ignored tests but fail anyway, for example this one:

2021-10-01T19:53:57.7418818Z TESTDONE: 1171 tests out of 1174 reported OK: 99%
2021-10-01T19:53:57.7419127Z 
2021-10-01T19:53:57.7419716Z TESTFAIL: These test cases failed: (203) (1056) (1143) 
2021-10-01T19:53:57.7420845Z 
2021-10-01T19:53:57.7462698Z make[1]: *** [nonflaky-test] Error 1
2021-10-01T19:53:57.7465251Z make[1]: Leaving directory `/c/__w/1/s/tests'
2021-10-01T19:53:57.7559396Z make: *** [test-nonflaky] Error 2
2021-10-01T19:53:57.8742529Z ##[error]Cmd.exe exited with code '2'. 

@bagder
Copy link
Member

bagder commented Oct 2, 2021

2021-10-01T19:53:57.7419716Z TESTFAIL: These test cases failed: (203) (1056) (1143)

That certainly looks like a test script bug!

@bagder
Copy link
Member

bagder commented Oct 12, 2021

@jay I believe my fixup addressed the remaining issue. Anything left or should this get merged?

- Make content length (ie download size) accessible to the user in the
  header callback, but only after all headers have been processed (ie
  only in the final call to the header callback).

Background:

For a long time the content length could be retrieved in the header
callback via CURLINFO_CONTENT_LENGTH_DOWNLOAD_T as soon as it was parsed
by curl.

Changes were made in 8a16e54 (precedes 7.79.0) to ignore content length
if any transfer encoding is used. A side effect of that was that
content length was not set by libcurl until after the header callback
was called the final time, because until all headers are processed it
cannot be determined if content length is valid.

This change keeps the same intention --all headers must be processed--
but now the content length is available before the final call to the
header function that indicates all headers have been processed (ie
a blank header).

Bug: curl@8a16e54#r57374914
Reported-by: sergio-nsk@users.noreply.github.com

Co-authored-by: Daniel Stenberg

Fixes curl#7804
Closes #xxxx
@jay jay force-pushed the make_contentlen_avail_in_headercb branch from a56502a to 8b55769 Compare October 13, 2021 06:46
@jay
Copy link
Member Author

jay commented Oct 13, 2021

Thanks, that seems to have done it. I've squashed it, rebased on master and will wait for another round of CI results.

@jay
Copy link
Member Author

jay commented Oct 14, 2021

All of the failures look like random flakiness, however one of them in windows msys1_mingw32_debug was test 566 which is content length related, so I'm rerunning that CI until it passes.

@jay jay closed this in b1d08d2 Oct 15, 2021
@jay jay deleted the make_contentlen_avail_in_headercb branch October 15, 2021 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

content length not available in header function
3 participants