Skip to content
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

Enable RuntimeProvider in DoT implementations #1373

Merged
merged 5 commits into from Feb 3, 2021

Conversation

chengyuhui
Copy link
Contributor

@chengyuhui chengyuhui commented Feb 3, 2021

I was surprised that RuntimeProvider does not work in DoT, which is not ideal for my application.

Most changes of this PR are just playing around with types, almost no changes have been made to the logic.

@codecov
Copy link

codecov bot commented Feb 3, 2021

Codecov Report

Merging #1373 (1fedd27) into main (6b2ed70) will increase coverage by 0.00%.
The diff coverage is 83.33%.

@@           Coverage Diff           @@
##             main    #1373   +/-   ##
=======================================
  Coverage   86.35%   86.35%           
=======================================
  Files         134      134           
  Lines       13870    13871    +1     
=======================================
+ Hits        11977    11978    +1     
  Misses       1893     1893           

@chengyuhui chengyuhui force-pushed the tls-provider branch 3 times, most recently from b50e7eb to f6b5091 Compare February 3, 2021 08:00
@chengyuhui
Copy link
Contributor Author

I keep getting this when running cargo make clippy locally (totally unrelated code halts the process), which is annoying.

image

@djc
Copy link
Collaborator

djc commented Feb 3, 2021

Are you using nightly Rust?

@chengyuhui chengyuhui force-pushed the tls-provider branch 2 times, most recently from b6a40ef to d751b41 Compare February 3, 2021 08:32
@chengyuhui
Copy link
Contributor Author

chengyuhui commented Feb 3, 2021

Oops, my bad. Now that's sorted out.

Having to convert between std and tokio does not look great, but we can't really do something if we want to maintain async-std compatibility?

Copy link
Collaborator

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes looks good in principe, though some of the type aliases look very complex. Does it really make sense to declare a TcpClientStream<AsyncIoTokioAsStd<TokioTlsStream<AsyncIoStdAsTokio<S>>>>?

In your changes, you changed the import style. I think these changes should be reverted/made to comply with the prevalent style. In the prevalent style, there is no nested grouping of imported items. I can make another pass once this is fixed.

@@ -26,9 +26,9 @@ use openssl::x509::*;
use futures_util::stream::StreamExt;
use rustls::Certificate;
use rustls::ClientConfig;
use tokio::runtime::Runtime;
use tokio::{net::TcpStream as TokioTcpStream, runtime::Runtime};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's put this on a separate line, in keeping with the surrounding style.

@chengyuhui
Copy link
Contributor Author

Well, I don't think anyone would like such a strange type. However, I currently have no idea how to simplify that.

TcpClientStream<AsyncIoTokioAsStd<TokioTlsStream<AsyncIoStdAsTokio<S>>>> where S: Connect
^                                 ^                                ^
std                               tokio                            std                                       

The issue is that each part of this type accepts different AsyncRead + AsyncWrite coming from the standard library (or futures) and tokio, which are incompatible.

@djc
Copy link
Collaborator

djc commented Feb 3, 2021

Ugh, yeah, I guess that's actually necessary.

@chengyuhui
Copy link
Contributor Author

I have updated all the styles.

Copy link
Collaborator

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine to me. Thanks!

@djc djc merged commit 26b842b into hickory-dns:main Feb 3, 2021
@chengyuhui
Copy link
Contributor Author

Please note that this would be a breaking change for every TLS implementation crate, as there is an extra type parameter.

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.

None yet

2 participants