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

HTTP CONNECT done non-blocking #1547

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@bagder
Member

bagder commented Jun 6, 2017

No description provided.

@mention-bot

This comment has been minimized.

mention-bot commented Jun 6, 2017

@bagder, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jay, @yangtse and @dfandrich to be potential reviewers.

@coveralls

This comment has been minimized.

coveralls commented Jun 6, 2017

Coverage Status

Coverage decreased (-0.03%) to 73.182% when pulling 5fec25b on bagder/HTTP-CONNECT-non-blocking into e100afb on master.

TUNNEL_INIT, /* init/default/no tunnel state */
TUNNEL_CONNECT, /* CONNECT has been sent off */
TUNNEL_COMPLETE /* CONNECT response received completely */
} tunnel_state; /* two separate ones to allow FTP */

This comment has been minimized.

@MarcelRaad

MarcelRaad Jun 6, 2017

Member

This comment is now wrong, isn't it?

This comment has been minimized.

@bagder

bagder Jun 6, 2017

Member

It is indeed! Thanks. The updated logic allocates a fresh struct for each CONNECT...

@coveralls

This comment has been minimized.

coveralls commented Jun 6, 2017

Coverage Status

Coverage increased (+0.05%) to 73.268% when pulling 5ba5ffa on bagder/HTTP-CONNECT-non-blocking into e100afb on master.

s->cl--;
infof(data,
"one byte down '%c', %" CURL_FORMAT_CURL_OFF_T " to go!\n",
*s->ptr, s->cl);

This comment has been minimized.

@jay

jay Jun 6, 2017

Member

Personally I'd lose this infof but at the very least wrap it in a DEBUGF

This comment has been minimized.

@bagder

bagder Jun 6, 2017

Member

Oops, yes that's a leftover from my debugging. I'll remove that completely, thanks!

@bagder bagder force-pushed the bagder/HTTP-CONNECT-non-blocking branch from 5ba5ffa to d0fdcbb Jun 6, 2017

http-proxy: do the HTTP CONNECT process entirely non-blocking
Mentioned as a problem since 2007 (8f87c15) and of course it
existed even before that.

@bagder bagder force-pushed the bagder/HTTP-CONNECT-non-blocking branch from d0fdcbb to 32eddae Jun 7, 2017

@coveralls

This comment has been minimized.

coveralls commented Jun 7, 2017

Coverage Status

Coverage decreased (-0.02%) to 73.206% when pulling 32eddae on bagder/HTTP-CONNECT-non-blocking into 68c6dcb on master.

@bagder

This comment has been minimized.

Member

bagder commented Jun 8, 2017

Just because this is a slightly larger change, I'm playing it safe and will not merge it this close to the next release. Scheduled for next instead.

@bagder bagder closed this in 5113ad0 Jun 14, 2017

@bagder bagder deleted the bagder/HTTP-CONNECT-non-blocking branch Jun 14, 2017

@dfandrich

This comment has been minimized.

Collaborator

dfandrich commented Jun 15, 2017

This commit broke builds with --disable-proxy or --disable-http

@bagder

This comment has been minimized.

Member

bagder commented Jun 15, 2017

Thanks, fixed now!

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