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

libcurl: stop reading from connection when client has paused receivin… #3240

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@dheerajsangamkar

dheerajsangamkar commented Nov 6, 2018

…g data

Goal: Allow libcurl pause-unpause behavior to control flow of data from server with limited memory consumption.

Setup: Using curl_multi_perform to transfer GBs of data from a server that is setup using a curl_easy_handle. (HTTP 1.1 and HTTP 2) When it is not possible to accept data, the WRITEFUNCTION returns CURL_WRITEFUNC_PAUSE. Curl stops sending data to the write function.

Problem:
Use curl_easy_pause(easy_handle, CURLPAUSE_CONT) in the main thread to 'unpause' the transfer that was paused earlier. The un-paused transfer starts and calls the WRITEFUNC again and after a few invocation gets CURL_WRITEFUNC_PAUSE. At this point, libcurl continues to read data from the connection and has to store it off of the easy handle because it cannot deliver the data to the WRITEFUNCTION that has paused. When reading GBs of data, this ultimately leads to Out of memory error from CURL and the transfer is aborted.

Root cause, in my opinion:
curl_multi_perform/curl_easy_pause ultimately calls lib/transfer.c:readwrite_data to read data off the connection from http server and delivers to WRITEFUNCTION using Curl_client_write. When WRITEFUNC requests pause, this state is set in the easy-handle but readwrite_data does not check this state and continues to read data from socket but cannot deliver to the WRITEFUNC.

Solution:
readwrite_data should check the KEEP_RECV_PAUSE flag in the easy handle and stop reading when it is set.

@dheerajsangamkar

This comment has been minimized.

dheerajsangamkar commented Nov 6, 2018

Related conversation on curl-library email list: https://curl.haxx.se/mail/lib-2018-11/0007.html

@bagder

bagder approved these changes Nov 6, 2018

I can only agree that this appears to be completely sensible. It's also a bit complicated to add a test for, so while the green CI tests are a good sign we also know that we don't have any tests for this particular code path.

@bagder

This comment has been minimized.

Member

bagder commented Nov 6, 2018

Thanks!

@bagder bagder closed this in 74f4782 Nov 6, 2018

@dheerajsangamkar

This comment has been minimized.

dheerajsangamkar commented Nov 7, 2018

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment