On 6/9/2014 6:01 PM, Daniel Stenberg wrote:
> On Sat, 7 Jun 2014, Ray Satiro wrote:
>
>> Returning a non-zero value from this callback will cause libcurl to
>> abort the transfer and return CURLE_ABORTED_BY_CALLBACK. That implies
>> to me at least that a progress function would not be called again if
>> it aborts.
>
> Thanks a lot for the detailed stack trace, it made me figure out a lot
> and I believe the attached patch is the correct fix.
>
> I'd be happy if either one of you could try it and verify that it does
> what I hope it does - I have not ran the test recipe myself!

The behavior didn't change. I applied your patch to e934439 2014-06-09.
After the progress callback aborts the first time Curl_done() is passed
status CURLE_OK. It looks like you're checking for status
CURLE_ABORTED_BY_CALLBACK at the line you modified so that
Curl_pgrsDone() won't be called in this case?

Highlights:

multi.c multi_runsingle() line 1704:
----------
       else if(data->easy_conn && Curl_pgrsUpdate(data->easy_conn)) {
----------

The user defined progress function is called the first time, fails,
CURLE_ABORTED_BY_CALLBACK is assigned to data->result, CURLM_STATE_DONE
is assigned to data->mstate, multi_runsingle() returns -1.

multi.c multi_socket() line 2284-2286:
----------
       do
         result = multi_runsingle(multi, now, data);
       while(CURLM_CALL_MULTI_PERFORM == result);
----------

Line 2285 is where multi_runsingle() returned -1 which is assigned to
result. Since CURLM_CALL_MULTI_PERFORM == -1, multi_runsingle() is
called again.

multi.c multi_runsingle() line 1612-1613:
----------
         /* post-transfer command */
         res = Curl_done(&data->easy_conn, CURLE_OK, FALSE);
----------

This is in the case instance for CURLM_STATE_DONE (recall that
data->mstate == CURLM_STATE_DONE).

url.c Curl_done() line 5794-5814:
----------
   switch(status) {
   case CURLE_ABORTED_BY_CALLBACK:
   case CURLE_READ_ERROR:
   case CURLE_WRITE_ERROR:
     /* When we're aborted due to a callback return code it basically
have to
        be counted as premature as there is trouble ahead if we don't.
We have
        many callbacks and protocols work differently, we could
potentially do
        this more fine-grained in the future. */
     premature = TRUE;
   default:
     break;
   }

   /* this calls the protocol-specific function pointer previously set */
   if(conn->handler->done)
     result = conn->handler->done(conn, status, premature);
   else
     result = CURLE_OK;

   if((status != CURLE_ABORTED_BY_CALLBACK) && Curl_pgrsDone(conn) &&
!result)
     result = CURLE_ABORTED_BY_CALLBACK;
----------

Since status == CURLE_OK, premature stays false (but should it..hm) and
then Curl_pgrsDone() is called.

Assuming you want to stop Curl_pgrsDone() the callback result is still
in data->result so maybe:

   if((data->result != CURLE_ABORTED_BY_CALLBACK) && Curl_pgrsDone(conn)
&& !result)

Likely that breaks other things though... and also it looks to me like
that would cover up what may be a bigger problem, which is why pass in
CURLE_OK on fail. I searched all the calls to Curl_done() and most look
like this:
Curl_done( .., data->result, .. ). So why is CURLE_OK passed instead of
data->result in the other cases (multi.c lines 1260, 1561, 1613)? Maybe
that's what's intended in all those cases but what would happen at least
in this case if your patch stays the same but also:

multi.c multi_runsingle() line 1613:
----------
- res = Curl_done(&data->easy_conn, CURLE_OK, FALSE);
+ res = Curl_done(&data->easy_conn, data->result, FALSE);
----------

After that change the progress callback is not called a final time after
CURLE_ABORTED_BY_CALLBACK. A side effect of that is in Curl_done()
premature will be true so Curl_disconnect() will be called and the
connection will not be reused (good or bad, I guess).

I wonder though what happens to the programmer's code if the progress
callback is no longer called a final time when
CURLE_ABORTED_BY_CALLBACK. Even if you want to prevent it in this case
what if now or in the future you have some other callback (ie not
CURLOPT_PROGRESSFUNCTION/CURLOPT_XFERINFOFUNCTION) that can return
CURLE_ABORTED_BY_CALLBACK, should the progress function be called a
final time in that case? Is anyone already relying on a final call to
the progress function, to get the final counts or something even if
fail? I don't know.

-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette: http://curl.haxx.se/mail/etiquette.html
Received on 2014-06-10