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

jpschroeder commented Nov 15, 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
Reported-by: @bdry

@jpschroeder jpschroeder force-pushed the jpschroeder:upload-stdin-nonblocking-hang branch from 4669808 to dd6e15c Nov 15, 2019
/* 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);

This comment has been minimized.

Copy link
@bagder

bagder Nov 15, 2019

Member

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! 😢

This comment has been minimized.

Copy link
@jpschroeder

jpschroeder Nov 15, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@jpschroeder

jpschroeder Nov 15, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@jpschroeder

jpschroeder Nov 17, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@jpschroeder

jpschroeder Nov 22, 2019

Author Contributor

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 jpschroeder:upload-stdin-nonblocking-hang branch 5 times, most recently from 27b68c8 to ff0be2c Nov 15, 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
Reported-by: @bdry
@jpschroeder jpschroeder force-pushed the jpschroeder:upload-stdin-nonblocking-hang branch from ff0be2c to ce685f3 Nov 22, 2019
@bagder
bagder approved these changes Nov 26, 2019
Copy link
Member

bagder left a comment

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 added 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

This comment has been minimized.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.