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

Treat an empty http_proxy mean "Don't use proxy" and skip parsing it #21632

Merged
merged 1 commit into from
Jan 17, 2020

Conversation

ikedam
Copy link
Contributor

@ikedam ikedam commented Jan 11, 2020

Replaces #21570

An empty value for http_proxy results error messages like this, as described in #17631 :

E1230 02:42:28.834924400       1 uri_parser.cc:46]           bad uri.scheme: ''
E1230 02:42:28.835090500       1 uri_parser.cc:52]                            ^ here
E1230 02:42:28.835106400       1 http_proxy.cc:63]           cannot parse value of 'http_proxy' env var

You can see this by simply running

$ http_proxy= bins/opt/h2_local_ipv4_test no_logging

I want to suppress this error as:

This change only suppress the error message and doesn't change the actual communication behavior.

@nicolasnoble

@@ -47,6 +47,7 @@ namespace {
*/
char* GetHttpProxyServer(char** user_cred) {
GPR_ASSERT(user_cred != nullptr);
grpc_uri* uri = nullptr;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To have new goto not cross the initialization.

@markdroth markdroth added kokoro:run lang/core release notes: yes Indicates if PR needs to be in release notes labels Jan 13, 2020
Copy link
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

This looks good to me. I've triggered the tests to run. I'll let Yash do the second review and merge when ready.

@yashykt
Copy link
Member

yashykt commented Jan 15, 2020

EasyCLA has not passed yet. @ikedam have you signed the cla?

@ikedam
Copy link
Contributor Author

ikedam commented Jan 16, 2020

I signed it.

@ikedam
Copy link
Contributor Author

ikedam commented Jan 16, 2020

I believe the above comment will proceed the check according to FAQ ( https://identity.linuxfoundation.org/projects/cncf ).

@ikedam
Copy link
Contributor Author

ikedam commented Jan 16, 2020

Hmm... the status doesn’t seem changed from “Expected — Waiting for status to be reported”.

I’ve already singed the CLA in #21570 .
I might need to sign again for each pull requests, but I haven’t received any Emails requesting that and I don’t know how to do that...

@yashykt yashykt closed this Jan 17, 2020
@yashykt yashykt reopened this Jan 17, 2020
@yashykt
Copy link
Member

yashykt commented Jan 17, 2020

Closing and reopening the PR did the trick

@yashykt
Copy link
Member

yashykt commented Jan 17, 2020

Unrelated interop build failures and other test issues - #19726, #21627

@yashykt yashykt merged commit 4466a4c into grpc:master Jan 17, 2020
@ikedam
Copy link
Contributor Author

ikedam commented Jan 18, 2020

Thanks!

@lock lock bot locked as resolved and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lang/core release notes: yes Indicates if PR needs to be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants