On Sat, 2008-08-02 at 22:37 +0200, Daniel Stenberg wrote:
> On Sat, 2 Aug 2008, Albert Lee wrote:
> ...
>
> > However, singlesocket() is immediately called after ???multi_runsingle(). If
> > the handle state was just changed to CURLM_STATE_DONE, singlesocket()
> > discards the sockets as described above - and since Curl_done() has not yet
> > been called the cleanup for those sockets never happens.
>
> But isn't this libcurl function then returning CURLM_CALL_MULTI_PERFORM (which
> your program should react on)?

Yes, the program (rtorrent again in this case) handles the return from
multi_socket correctly.

What I was worried about was that multi_runsingle needed to be called
twice *before* multi_socket returned, to keep singlesocket from
modifying the socket table. It is now apparent that the singlesocket
behaviour is correct and the problem is elsewhere.

>
> And further, hasn't it already set a timeout at the end of multi_runsingle()
> that expires after 1 millisecond that also would cause curl_multi_socket*() to
> get called on that particular file descriptor?
>
> I think the key to this problem is to understand why your app isn't calling
> libcurl again so that it drives the state forward to COMPLETED instead of
> letting it stay in DONE. Or if it indeed does call it, why that isn't enough.
>
> > Making ???singlesocket() not modify the socket hash table if the handle is
> > in ??????CURLM_STATE_DONE seems to fix the problem - the sockets can be
> > removed in a subsequent multi_socket() called at which point the handle will
> > be in CURLM_STATE_COMPLETED and the sockets will actually have been closed.
>
> That seems like a band-aid fix that is not very nice. singlesocket() should be
> state-agnostic and there should never be a reason to just "skip" a state. If
> the DONE state would need a special set of sockets then those sockets should
> be set in the array by the multi_getsock() function IMO.
>

Just giving our timeout callback a kick via debugger when curl has
gotten stuck allows libcurl to continue normally, so indeed the internal
handling of the DONE state was correct, and timeouts appear to be
malfunctioning.

It appears that our curl stack was actually not handling timeouts
correctly - it is not entirely clear from the documentation,
and unfortunately several other consumers of libcurl have
misinterpreted it as well. We used on the CURLM_TIMERFUNCTION callback
to synchronously schedule timeouts for us, and so we completely forgot
the timeout when the value didn't change.

The documentation should probably state that timeouts should be
scheduled asynchronously outside the callback (unlike
curl_multi_timeout), and specify whether -1 should be handled the same
way as for curl_multi_timeout (that is, set an arbitrarily long timeout
instead of no timeout). BTW, the curl_multi_timeout manual still refers
to an nonexistant 'easy' argument for curl_multi_socket.

I'm guessing timeouts should be scheduled in the same part of the event
loop where one would normally call curl_multi_timeout, i.e. right before
we listen for new events. Unfortunately our event loop is totally
isolated from the curl code, which is only invoked when handling events
on specific sockets. I am considering making the function that calls
curl_multi_socket_action re-schedule the timeout each time before
returning, but it's less efficient since may be called dozens of times
in a single event loop iteration.

-Albert
Received on 2008-08-06