cURL / Mailing Lists / curl-library / Single Mail

curl-library

RE: hangs up of application above libcurl

From: Igor Novoseltsev <IgorN_at_radvision.com>
Date: Tue, 16 Dec 2008 16:21:37 +0200

> There's one little detail that nudges me about this patch. Why are the
> !isHandleAtHead() checks needed? Clearly if the socket is used for a
> pipeline,
> meaning that at least one other handle is also using the same socket,
then
> surely the socket shouldn't be removed from the app no matter which of
the
> handles that are head at that given moment? I mean isn't it enough to
spot
> that it is part of a pipeline and thus used by another handle as well
and
> then
> it shouldn't be removed (yet)?

You are completely right.
I tried to handle situation, where the subject handle is in the pipe.
The fixed patch is attached.

> Also, to "repair" (setting the easy field of the entry struct to the
head of
> a pipe queue) the sockhash entry at that point seems a bit
ineffective,

I didn't mean to "repair" the sockhash entry.
I meant "remove" reference to handle stored in the sockhash entry,
because the handle is going to be freed. This will make the sockhash
entry unusable anymore.
You can see the idea more clearly in the fixed patch.

> if not
> plain wrong, since then multi_socket() might already have ran on the
wrong
> SessionHandle.

The multi_socket() should not run on wrong handle if we apply the fixed
patch.
The fix will cause the multi_socket to get the handle from the head of
pipe.

> But then I'm also getting a slight head ache when I consider the fact
that
> there are pipelines in both directions on the same socket so the
"right"
> SessionHandle also depends on what direction the "action" is at that
point.

I fixed the patch in order to implement "when we call libcurl to "act"
on a
socket, we should make an effort to work with the easy handle that is at
the
head of that pipeline ". If the fix works OK, there is no need to bother
with
the direction of pipelines on "repairing".

> A minor nit is also that 'rename' is not a good name for a variable
(it
> shadows a global symbol), and if you build after having run configure
> --enable-debug you'd see the warning for it!

I guess you meant 'remove'. I replaced it with 'removeSockFromHash'.
See the attached fixed patch.

> I think we need to consider this, yes. When we call libcurl to "act"
on a
> socket, we should make an effort to work with the easy handle that is
at the
> head of that pipeline (assuming the socket is used for pipelining of
course).

The attached fixed patch includes this fix.

While fixing the patch I used following assumption:
If multi_socket got the WRITE_INTO_SOCKET (ev_bitmask & CURL_POLL_OUT)
action,
the handle from the send_pipe head should be taken,
otherwise - from the recv_pipe.

Note this assumption can be broken, if the OpenSSL is used for TLS.
From my experience with the OpenSSL I know, that when renegotiation
is run on the connection, the OpenSSL may require POLL_OUT condition
in order to complete READ operation, and POLL_IN in order to complete
writing into socket.
I didn't dig enough into the libcurl tls in order to check if this is
possible.
Meanwhile I am happy with the assumption.

Note I fail to reproduce the original hanging,
therefore I can't verify the patch right now.
If we agree about the patch I'll try to downgrade
the libcurl version in order to reproduce the hang.

Received on 2008-12-16