Skip to content

Conversation

vadz
Copy link
Contributor

@vadz vadz commented Jul 7, 2017

This PR consists of 2 somewhat independent parts (I decided it would be more convenient to have them in the same PR, but please let me know if you want me to split it in 2): first, and most importantly, the first commit fixes the sample behaviour by default as currently it just fails to open connection if http_proxy is not defined, so it's broken out of the box. This is especially annoying because there is no exception handling in the sample (which is IMO far from ideal, but I didn't change this because this is, perhaps, intentional to keep things simple?), so it simply crashes without any explanation as to why does it happen.

Second, the 2nd and 3rd commits allow to use http_proxy=auto with this sample as it's convenient for testing.

Using empty proxy URI results in failing to open the connection when using
WinHTTP, so the current code is broken out of the box.
@msftclas
Copy link

msftclas commented Jul 7, 2017

@vadz,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

vadz added 2 commits July 8, 2017 13:19
The length returned by _wdupenv_s() includes the terminating NUL character,
which isn't really part of the string.

URI parsing seems to discard it anyhow currently (intentionally or not?), but
including it breaks comparisons with any literal strings, for example, such as
are done in the upcoming commit.
Allow easily testing proxy auto-discovery using this sample too.
@vadz vadz changed the title Don't use proxy in BingRequest sample if not defined Improve http_proxy handling in BingRequest sample Jul 8, 2017
@ras0219-msft
Copy link
Contributor

This seems good to me, thanks for the PR!

@ras0219-msft ras0219-msft merged commit 8833a2b into microsoft:master Aug 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants