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

try to fix CONNECT blocking #703

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@maxim-ky

maxim-ky commented Mar 7, 2016

Hello, guys!

I use curl multi-* API and as far as I understand it's supposed to be non-blocking. But I have an issue with HTTP proxy: during querying the proxy with CONNECT method (establishing a tunnel for an HTTPS session) libcurl blocks in curl_multi_socket_action until the proxy returns "200 Connection established". After some debugging I've found that actually libcurl blocks in multi_runsingle function. The code snippet below explains the details:

do {
  switch( state ) {
    case CURLM_STATE_WAITPROXYCONNECT:
       // this unconditional statement causes that blocking
       rc = CURLM_CALL_MULTI_PERFORM;
    break;
  };
} while( rc == CURLM_CALL_MULTI_PERFORM );

It seems we can not to block in the loop but wait a callback from curl_multi_socket_action.
Here is the patch. It's working for me but still it looks pretty naive :) Please review it and give me a hint how to solve the faced issue in a proper way.
I'm looking forward to hearing from you soon.

Regards

@bagder bagder self-assigned this Mar 8, 2016

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Mar 8, 2016

Member

Thanks! Gosh, that looks like a silly code path and my first gut reaction says your patch looks right. Let me just run some tests of my own first before I merge...

Member

bagder commented Mar 8, 2016

Thanks! Gosh, that looks like a silly code path and my first gut reaction says your patch looks right. Let me just run some tests of my own first before I merge...

@frenche

This comment has been minimized.

Show comment
Hide comment
@frenche

frenche Mar 8, 2016

Contributor

Note that this issue is mentioned in libcurl-multi man page.

Contributor

frenche commented Mar 8, 2016

Note that this issue is mentioned in libcurl-multi man page.

@maxim-ky

This comment has been minimized.

Show comment
Hide comment
@maxim-ky

maxim-ky Mar 8, 2016

Thanks! Gosh, that looks like a silly code path and my first gut reaction says your patch looks right. Let me just run some tests of my own first before I merge...

I have some doubts about connetion timeouts. They were handled in a blocking manner in Curl_proxyCONNECT, but now are they okay?

Note that this issue is mentioned in libcurl-multi man page.

Oh, it really is! I was looking for it on the known bugs page, but never on the man page. I'm sorry.
Now it seems it's a well-known issue, so the patch is a way more suspicious now (it looks too simple to fix an old issue like that).

maxim-ky commented Mar 8, 2016

Thanks! Gosh, that looks like a silly code path and my first gut reaction says your patch looks right. Let me just run some tests of my own first before I merge...

I have some doubts about connetion timeouts. They were handled in a blocking manner in Curl_proxyCONNECT, but now are they okay?

Note that this issue is mentioned in libcurl-multi man page.

Oh, it really is! I was looking for it on the known bugs page, but never on the man page. I'm sorry.
Now it seems it's a well-known issue, so the patch is a way more suspicious now (it looks too simple to fix an old issue like that).

@frenche

This comment has been minimized.

Show comment
Hide comment
@frenche

frenche Mar 8, 2016

Contributor

Note that this issue is mentioned in libcurl-multi man page.
Oh, it really is! I was looking for it on the known bugs page, but never on the man page. I'm sorry.
Now it seems it's a well-known issue, so the patch is a way more suspicious now (it looks to simple to fix an old issue like that).

I don't really know the history of it, but it'll definitely be nice to
see it fixed :)

Contributor

frenche commented Mar 8, 2016

Note that this issue is mentioned in libcurl-multi man page.
Oh, it really is! I was looking for it on the known bugs page, but never on the man page. I'm sorry.
Now it seems it's a well-known issue, so the patch is a way more suspicious now (it looks to simple to fix an old issue like that).

I don't really know the history of it, but it'll definitely be nice to
see it fixed :)

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Mar 8, 2016

Member

Right, I wrote both the code and the section in the man page so I'm actually aware of that. =)

The blocking nature of the CONNECT procedure in libcurl has been slightly improved over time and it is much less blocking these days but even if this particular fix is fine (I haven't yet done the necessary testing myself) it does not make all CONNECT uses fully non-blocking so it should probably still be mentioned there.

Member

bagder commented Mar 8, 2016

Right, I wrote both the code and the section in the man page so I'm actually aware of that. =)

The blocking nature of the CONNECT procedure in libcurl has been slightly improved over time and it is much less blocking these days but even if this particular fix is fine (I haven't yet done the necessary testing myself) it does not make all CONNECT uses fully non-blocking so it should probably still be mentioned there.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Mar 8, 2016

Member

I have some doubts about connection timeouts. They were handled in a blocking manner in Curl_proxyCONNECT, but now are they okay?

I don't think there's any connection timeout problem either with or without this suggested change. But I imagine you can test that fairly easily.

Member

bagder commented Mar 8, 2016

I have some doubts about connection timeouts. They were handled in a blocking manner in Curl_proxyCONNECT, but now are they okay?

I don't think there's any connection timeout problem either with or without this suggested change. But I imagine you can test that fairly easily.

@maxim-ky

This comment has been minimized.

Show comment
Hide comment
@maxim-ky

maxim-ky Mar 8, 2016

I don't think there's any connection timeout problem either with or without this suggested change. But I imagine you can test that fairly easily.

Of course I'll check it manually. But what do you think: is it worth it to make some tests which will cover the discussed CONNECT behaviour?

maxim-ky commented Mar 8, 2016

I don't think there's any connection timeout problem either with or without this suggested change. But I imagine you can test that fairly easily.

Of course I'll check it manually. But what do you think: is it worth it to make some tests which will cover the discussed CONNECT behaviour?

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Mar 8, 2016

Member

it'd be great to have such a test, but it is probably not that easy to make

Member

bagder commented Mar 8, 2016

it'd be great to have such a test, but it is probably not that easy to make

@maxim-ky

This comment has been minimized.

Show comment
Hide comment
@maxim-ky

maxim-ky Mar 9, 2016

But I imagine you can test that fairly easily.

Done! It works because multi_runsingle checks the connection timeout with Curl_timeleft. Sorry for the false alarm :)

it'd be great to have such a test, but it is probably not that easy to make

I'll try to make one. At least it can check that blocking behavior in CONNECT.

maxim-ky commented Mar 9, 2016

But I imagine you can test that fairly easily.

Done! It works because multi_runsingle checks the connection timeout with Curl_timeleft. Sorry for the false alarm :)

it'd be great to have such a test, but it is probably not that easy to make

I'll try to make one. At least it can check that blocking behavior in CONNECT.

@bagder bagder closed this in d7e3942 Mar 13, 2016

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