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

HTTP CONNECT done non-blocking #1547

Closed
wants to merge 1 commit into from

Conversation

bagder
Copy link
Member

@bagder bagder commented Jun 6, 2017

No description provided.

@mention-bot
Copy link

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

Coverage Status

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

lib/urldata.h Outdated
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 */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is now wrong, isn't it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@coveralls
Copy link

Coverage Status

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

lib/http_proxy.c Outdated
s->cl--;
infof(data,
"one byte down '%c', %" CURL_FORMAT_CURL_OFF_T " to go!\n",
*s->ptr, s->cl);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Compare June 6, 2017 22:51
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 Compare June 7, 2017 21:02
@coveralls
Copy link

Coverage Status

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

@bagder bagder added the feature-window A merge of this requires an open feature window label Jun 8, 2017
@bagder
Copy link
Member Author

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 June 14, 2017 21:45
@dfandrich
Copy link
Contributor

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

@bagder
Copy link
Member Author

bagder commented Jun 15, 2017

Thanks, fixed now!

@bagder bagder removed the feature-window A merge of this requires an open feature window label Sep 5, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Dec 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

None yet

6 participants