Skip to content
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

Jump to bottom

try to fix CONNECT blocking #703

Closed
wants to merge 1 commit into from
Closed

try to fix CONNECT blocking #703

wants to merge 1 commit into from

Conversation

maxim-ky
Copy link

@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
Copy link
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...

@iboukris
Copy link
Contributor

iboukris commented Mar 8, 2016

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

@maxim-ky
Copy link
Author

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).

@iboukris
Copy link
Contributor

iboukris 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
Copy link
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
Copy link
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
Copy link
Author

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
Copy link
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
Copy link
Author

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
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@bagder bagder

Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@maxim-ky @bagder @iboukris