bitreq: TLS modules refactor#569
Conversation
ton-anywhere
requested review from
TheBlueMatt,
oleonardolima and
tnull
as code owners
|
CI failing because it's treating |
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
Hmm, rather than an indirect feature can we gate on the actual features we need?
| use std::sync::OnceLock; | ||
|
|
||
| #[cfg(all(feature = "native-tls", not(feature = "rustls")))] | ||
| #[cfg(all(feature = "https-native-tls", not(feature = "_https-rustls")))] |
There was a problem hiding this comment.
Can we gate this whole module on rustls so this isn't so much of a mess, or at least move to mod rustls_stuff { ... }; pub use rustls_stuff::*?
|
@TheBlueMatt you mean using |
|
Yea, that is what I was thinking. |
ton-anywhere
force-pushed
the
tls-feature-gates
branch
from
d866da0 to
9cb51d8
Compare
|
@TheBlueMatt updated |
| all( | ||
| feature = "https-native-tls", | ||
| not(any(feature = "https-rustls", feature = "https-rustls-probe")) | ||
| ), |
There was a problem hiding this comment.
I don't think we intend to allow selection of a separate sync-tls-provider and async one.
There was a problem hiding this comment.
What is your suggestion?
The alternatives I came up were either convoluted or do not pass in tests like this one:
cargo --locked build --no-default-features "--features=async-https-native-tls https async-https-rustls native-tls rustls rustls-webpki std tokio webpki-roots"
I was using this and it failed:
#[cfg[all(
any(feature = "async-https-native-tls", feature = "https-native-tls"),
not(any(feature = "https-rustls", feature = "https-rustls-probe"))
)]
Because it was opening all async gates(async backend present) but rustls priority was taking over.
The simplest solution I can think of without a large rework on all async gates(many) would be just ignore sync on priorities when an async feature is on. But that would change in behavior we intended(rustls priority over native-tls). Otherwise we could simply not compile, but that's again a change in behavior.
But, I bet most of the users won't mix with both configs at the same time. So if they set a async feature it will be set for both sync/async. The gate was necessary to work on edge cases like the one above..
| feature = "https-native-tls", | ||
| not(any(feature = "https-rustls", feature = "https-rustls-probe")) | ||
| ))] | ||
| use self::native_tls_stream as sync_tls_stream; |
There was a problem hiding this comment.
Its probably simpler to just have a sub-module in rustls_stream (maybe renaming it) and re-export. That way there's a single API to connection.rs and we arent conditional-compiling which module to use here.
There was a problem hiding this comment.
really? using a single file to store code for both backends is a missing opportunity IMO. You mean like the enabled/disabled stuff I've initially tough for the tls configs on client.rs? That's doable, but I don't see what's are we winning here.
| Ok(HttpStream::Secured(Box::new(tls), None)) | ||
| } | ||
|
|
||
| // === ASYNC native-tls === |
There was a problem hiding this comment.
Please don't commit claude's useless code section definition comments for a tiny file.
There was a problem hiding this comment.
@TheBlueMatt come on, do you even read what you write? I'm trying to help here but it's been really hard to work with you.
You commited the same type o comment in the master. this one is no different:
https://github.com/rust-bitcoin/corepc/blame/46c7bf4284748ee3da549d4463841e67db3bd00e/bitreq/src/connection/rustls_stream.rs#L69
I don't even use the fking Claude. I'm spending hours on all those pull requests. Come on. Let's be a bit more professionals here.
I agree 100% about removing all those useless comments, but this is the useless pattern you've already used there..
2 participants
Similar to this pull request, I encountered an error using the
stdandrustlsfeatures together.This PR makes TLS backend selection explicit by using cfg gates with cargo features instead of relying on implicit dependency features.
While working on mixed TLS feature combinations, I ran into cases where backend selection became unclear so I had to decouple sync and async modes.
This PR:
rustls_streamandnative_tls_streamrustlsprecedence overnative-tlswhen both backends are enabled for the same sync or async modeError Example