Skip to content

Conversation

@garethsb
Copy link
Contributor

No description provided.

@BillyONeal
Copy link
Member

I'm somewhat concerned with making the http_version field public because the other backends don't understand that, and callers could get the idea that we're going out of our way to support a 1.0 (rather than 1.1) mode. Perhaps some friends are in order?

There's also a concern here that this theoretically could expose one connection in the connection pool to both HTTP versions; but the version probably should probably be consistent across a connection.

@garethsb
Copy link
Contributor Author

garethsb commented Jan 28, 2019

Thanks for the review. I wasn't intending to make it any more public than it was - just add it to the internal implementation of http_response as well as http_request. It's still not exposed from the actual user-facing http_response class.

@garethsb
Copy link
Contributor Author

Regarding this point:

There's also a concern here that this theoretically could expose one connection in the connection pool to both HTTP versions; but the version probably should probably be consistent across a connection.

I don't have a particular opinion on this, except that if a client makes an HTTP/1.1 request, it may get a HTTP/1.0 response, so the connection changes at that point at least?

@garethsb
Copy link
Contributor Author

@BillyONeal If there any particular changes that would enable you to merge this to master, please let me know.

It would be possible to avoid storing the http_version in the internal _http_response class and instead pass it through the asio_context::read_headers call, though I prefer the current PR version at the moment.

Thanks

@BillyONeal
Copy link
Member

Looking at some of the types here again, it looks like we're already leaking internal implementation details all over the place prefixed with a _leading_underscore, so I guess this doesn't change the current status quo.

The remaining non-flaky test failure is from an Android SDK update that got pushed on us by Azure Pipelines I'm trying to fix in a separate PR.

Thanks for your contribution!

@BillyONeal BillyONeal merged commit f940d55 into microsoft:master Feb 11, 2019
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.

2 participants