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

curl: fix --upload-file . hangs if delay in STDIN #4599

Closed

Conversation

jpschroeder
Copy link
Contributor

Attempt to unpause a busy read in the CURLOPT_XFERINFOFUNCTION.

When uploading from stdin in non-blocking mode, a delay in reading
the stream (EAGAIN) causes curl to pause sending data
(CURL_READFUNC_PAUSE). Prior to this change, a busy read was
detected and unpaused only in the CURLOPT_WRITEFUNCTION handler.
This change performs the same busy read handling in a
CURLOPT_XFERINFOFUNCTION handler.

Fixes: #2051
Reported-by: @bdry

/* when reading from stdin in non-blocking mode, we use the progress
function to unpause a busy read */
my_setopt(curl, CURLOPT_XFERINFOFUNCTION, tool_readbusy_cb);
my_setopt(curl, CURLOPT_XFERINFODATA, per);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not happy with how this will

  1. disable the regular default progress bar
  2. disable the --progress-bar option if set
  3. Not work for parallel transfers

... but I also don't have a very good answer on how to do this instead! 😢

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely agree. I'm happy to explore other approaches, but I couldn't think of a better way. I wanted to put this out there to see what you guys thought.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding 2 above, It will actually honor the --progress-bar option. I just pushed an update that adds the same busy read checking into the regular CURLOPT_XFERINFOFUNCTION handler also.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding 3 above. I added the busy read checking to the parallel CURLOPT_XFERINFOFUNCTION handler also.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding 1 above. I added a return code to the progress callback that will cause libcurl to continue executing the default progress function.

@jpschroeder jpschroeder force-pushed the upload-stdin-nonblocking-hang branch 5 times, most recently from 27b68c8 to ff0be2c Compare November 22, 2019 05:10
Attempt to unpause a busy read in the CURLOPT_XFERINFOFUNCTION.

When uploading from stdin in non-blocking mode, a delay in reading
the stream (EAGAIN) causes curl to pause sending data
(CURL_READFUNC_PAUSE).  Prior to this change, a busy read was
detected and unpaused only in the CURLOPT_WRITEFUNCTION handler.
This change performs the same busy read handling in a
CURLOPT_XFERINFOFUNCTION handler.

Fixes: curl#2051
Reported-by: @bdry
Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the CURL_PROGRESSFUNC_CONTINUE idea!

@bagder bagder self-assigned this Nov 26, 2019
@bagder bagder closed this in 7cf18b0 Nov 26, 2019
bagder pushed a commit that referenced this pull request Nov 26, 2019
Attempt to unpause a busy read in the CURLOPT_XFERINFOFUNCTION.

When uploading from stdin in non-blocking mode, a delay in reading
the stream (EAGAIN) causes curl to pause sending data
(CURL_READFUNC_PAUSE).  Prior to this change, a busy read was
detected and unpaused only in the CURLOPT_WRITEFUNCTION handler.
This change performs the same busy read handling in a
CURLOPT_XFERINFOFUNCTION handler.

Fixes #2051
Closes #4599
Reported-by: bdry on github
@bagder
Copy link
Member

bagder commented Nov 26, 2019

Thanks!

As you'll see, I split your commit in two: one for the library change and one for the tool change, before I merged.

@lock lock bot locked as resolved and limited conversation to collaborators Feb 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

curl --upload-file . hang if delay in STDIN
2 participants