Code review comment for lp://staging/~gary-wzl77/net-cpp/bug-fixing-and-features

Revision history for this message
Gary.Wang (gary-wzl77) wrote :

Thanks, Please add the following test case in http_streaming_client_test.cpp.
http://bazaar.launchpad.net/~gary-wzl77/+junk/net-cpp_test_pause_and_resume/revision/53/tests/http_streaming_client_test.cpp#tests/=

Then we can see the potential issue we have in this MP after it is run. (I use the async api to reproduce this issue.)

Log:
....
Download progress: 0.0154666
Download progress: 0.0154666
we pause
Download progress: 0.0154666
Download progress: 0.0154666
Download progress: 0.0154666
we resume.

I resume download after the request is paused for 5 secs. But there is no "Download progress" log printing out any more even if I resume it. It causes the curl thread to hang as well. The issue can be reproduces 100%.

However if I explicitly set async_abort_if(1k/s, 10s) before request asynchronous execution(async_execute). Everything works fine.

Log:
Download progress: 0.0077333
Download progress: 0.0077333
Download progress: 0.0077333
we pause
Download progress: 0.0077333
we resume.
Download progress: 0.0077333
Download progress: 0.0077333
Download progress: 0.0077333
Download progress: 0.00883806
Download progress: 0.00883806

So it's better to mention this case in doc somewhere to reminder developer of setting abort_request_option explicitly before request asynchronous execution if they really want to pause/resume a request in one connection.
Otherwise developer doesn't figure out why it doesn't work out after resuming from paused state.
But anyway, it works fine if developers don't use pause/resume API.

P.S:
curl utilizes progress callback(CURLOPT_PROGRESSFUNCTION) to set bitmask(CURLPAUSE_CONT/CURLPAUSE_ALL) for a connection state change(pause/resume). However the default value of speed_time(2nd argument for abort_request_if) is 0(disabled). So after request is paused, progress cb gets no chance to call in such a case. It failed to resume even if we set the bitmask(CURLPAUSE_CONT).

« Back to merge proposal