-
Notifications
You must be signed in to change notification settings - Fork 49
Proxy h2 #402
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
Conversation
graebm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some trivial feedback, but looks good overall
| bool prior_knowledge_http2; | ||
| struct aws_http1_connection_options original_http1_options; | ||
| struct aws_http2_connection_options original_http2_options; /* allocated with userdata */ | ||
| struct aws_hash_table *alpn_string_map; /* allocated with userdata */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it would be less confusing to just store the alpn_string_map by value? instead of the weird allocate_many thing?
struct aws_hash_table alpn_string_map;
instead of
struct aws_hash_table *alpn_string_map;
It's safe to call our aws_XYZ_clean_up() functions on something that you never called init() on, as long as it's zeroed out
| size_t original_initial_window_size; | ||
| bool prior_knowledge_http2; | ||
| struct aws_http1_connection_options original_http1_options; | ||
| struct aws_http2_connection_options original_http2_options; /* allocated with userdata */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure why /* allocated with userdata */ comment is on original_http2_options which is a by-value thing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The options has setting array, which need to be allocated from heap. The comment was for that, will clarify it more.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #402 +/- ##
=======================================
Coverage ? 41.80%
=======================================
Files ? 859
Lines ? 128280
Branches ? 18041
=======================================
Hits ? 53633
Misses ? 70779
Partials ? 3868 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Issue #, if available:
Description of changes:
:authoritywithhost, I am totally okay with what ever the real world want. :)TODO:
The
AWS_TEST_HTTPS_H2_PROXY_URLnow has a proxy host that can talk http/2 with client. Used apache server with mod_proxy_http2. You can simply use that host for the proxy option and update the tls of proxy options to have h2 in the alpn string to create the h2 connection with proxy. However, I don't know what's the expected behavior for client to talk to a h2 proxy host.curldoesn't seem supporting it.So, put it as TODO, but still have the host here incase we want to implement it in the future.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.