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

max_idle_timeout config setting #368

Closed
ealione opened this issue Jan 31, 2022 · 3 comments
Closed

max_idle_timeout config setting #368

ealione opened this issue Jan 31, 2022 · 3 comments
Assignees
Labels
bug Something isn't working

Comments

@ealione
Copy link

ealione commented Jan 31, 2022

Not necessarily a bug, but I just wanted to make sure since I'm new in Rust. On line 218, function max_transport_config, file configs.rs. the code looks like this:

let _ = config.max_idle_timeout(Some(idle_timeout)).ok();

But max_idle_timeout's documentation, when it comes to Durations, states that it should look more like this:

let _ = config.max_idle_timeout(Some(idle_timeout).try_into()?);

With the change above Pycharm does not complain anymore for mismatched_types. As I said, not a bug, just looking to discuss this with someone that knows better.

@ealione ealione added the bug Something isn't working label Jan 31, 2022
@ealione
Copy link
Author

ealione commented Feb 5, 2022

Another issue, in connection.rs line 462 we have the line

warn!("Error handling endpoint echo request: {}", error);

The error here should be a SendError correct?
But if that is the case, why does it not implement Display?

@ealione
Copy link
Author

ealione commented Mar 17, 2022

I can't create a new issue for some reason right now. But I additionally wanted to mention:

Would using serde_bytes help speed up serializing/de-serializing in cases like wire_msg?
For example in places like let mut data: Vec<u8> = vec![0; data_length];

@maqi
Copy link
Member

maqi commented Jul 6, 2022

Hi, @esmeraldaliaj , thank you for the effort to raise the issue and really sorry for the late reply.

when it comes to Durations, states that it should look more like this

I checked the documentation of quinn, the API of max_idle_timeout changed from taking Option<Duration> to Option<IdleTimeout> around version 7.0 or 8.0 . That explains why you seeing this mismatch error.
We will refactor the code correspondently during next round of updating dependent crates versions.
Thanks for the alert.

The error here should be a SendError correct?

Yes

But if that is the case, why does it not implement Display?

Well, it's just following the quinn stuff to implement the required stuff. The errors from quinn related to this doesn't have Display implemented either, and we don't have extra requirement for Display, so it doesn't implements Display yet.
We will implement it once we need it.

Would using serde_bytes help speed up serializing/de-serializing in cases like wire_msg?

Yes, serde_bytes has been used for some small structs already.
For the bit more complicated WireMsg, we choose to use bytes crate which provides more facility in addition to the speedy serializing/de-serializing.

Thanks again for the effort. I am currently closing the issue.
You are welcomed to raise any further comment for discussion or reopen the issue if necessary.

@maqi maqi closed this as completed Jul 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants