-
Notifications
You must be signed in to change notification settings - Fork 17
Fix: h2 tls #57
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
base: master
Are you sure you want to change the base?
Fix: h2 tls #57
Conversation
|
wu_jia_tong seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
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.
There's also some detail I don't know. @har23k could you please also have a look at it?
| .collect(); | ||
|
|
||
| encoder.encode(&mut input.clone().into_iter(), &mut buf); | ||
| encoder.encode(input.clone().into_iter(), &mut buf); |
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.
What's the difference here?
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 borrowed expression implements the required traits
for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrows_for_generic_args
#[deny(clippy::needless_borrows_for_generic_args)] implied by #[deny(warnings)]
a clippy warning, you can change it back.
|
|
||
| // (next-state, byte, flags) | ||
| pub const DECODE_TABLE: [[(usize, u8, u8); 16]; 256] = [ | ||
| pub static DECODE_TABLE: [[(usize, u8, u8); 16]; 256] = [ |
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.
If we won't change it, put it in .data is better than .bss?
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.
large array defined as const
for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#large_const_arrays
#[warn(clippy::large_const_arrays)] on by defaultclippy[Click for full compiler diagnostic](rust-analyzer-diagnostics-view:/diagnostic message [0]?0#file:///workspaces/bw/pr/monoio-http/monoio-http/src/h2/hpack/huffman/table.rs)
table.rs(265, 5): make this a static item: static
large array defined as const
for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#large_const_arrays
#[deny(clippy::large_const_arrays)] implied by #[deny(warnings)]clippy[Click for full compiler diagnostic](rust-analyzer-diagnostics-view:/diagnostic message [2]?2#file:///workspaces/bw/pr/monoio-http/monoio-http/src/h2/hpack/huffman/table.rs)
mod.rs(80, 24): the lint level is defined here
table.rs(265, 5): make this a static item: static
this is my editor's warning, you can change it back.
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.
Great! I did some research and you are right.
With const: it will be replaced and may cause big output;
With static: it not always be compiled to .bss, when it is mutable and uninitialized, it would be put into .bss; otherwise it would be put into .rodata/.data. Here .rodata would be used.
const CONST_ARRAY: [u8; 2] = [1, 2];
static STATIC_ARRAY: [u8; 2] = [1, 2];
static mut STATIC_MUT_ARRAY: [u8; 2] = [1, 2];I wrote a demo, and objdump the target. I can found STATIC_ARRAY in .rodata, and STATIC_MUT_ARRAY in .data, CONST_ARRAY symbol cannot be found.
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.
Thanks for your detailed research! 😊
| // #[cfg(feature = "rustls")] | ||
| // assert_eq!(key.server_name, Some("bytedance.com".try_into().unwrap())); | ||
| // #[cfg(all(feature = "native-tls", not(feature = "rustls")))] | ||
| // assert_eq!(key.server_name, Some("bytedance.com".into())); |
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.
Why is this commented out?
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.
Could you please explain it? And if you meant to delete, please not use comment.
|
|
||
| pub trait NewTlsStream { | ||
| fn new(config: ConnectionConfig) -> Self; | ||
| } |
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.
How about require From<ConnectionConfig> instead of defining a new trait?
|
Sorry I didn't noticed the PR updating. I'm happy to have people contribute code to this project and would be happy to merge it. Apart from the slow review (which is my fault), is there any other reason why this PR was closed? |
| .http2_max_concurrent_streams(200) | ||
| .build(); | ||
|
|
||
| let base_url = "https://api.binance.com"; |
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.
I suggest using some neutral service like httpbin. I'm not sure if there are some policy issues here.
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.
Hi, @ihciah I’ve addressed the previous feedback and updated the code. Could you please review it again when you have time?
Fix TLS issue in the origin HTTP client
The origin HTTP client was experiencing problems with TLS, causing failures when making secure requests. This commit updates the TLS configuration to handle h2 connenction correctly.
more specifitily this code was unable to run before.