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

Fix: replace is_connect_failed with is_err #189

Merged
merged 56 commits into from
Mar 21, 2024

Conversation

irvingoujAtDevolution
Copy link
Contributor

In linux, epoll, EPOLLHUP may happen even if no connection call is made. It would confuse callers for what is actually happening.
Replaced is_connect_failed, and we detect if connection failed by using the combination of is_err and is_interrupt, please see the example, tcp_client

Copy link
Member

@notgull notgull left a comment

Choose a reason for hiding this comment

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

Unfortunately we can't change is_connect_failed as that would be a breaking change. So we have to keep the name the same there.

@irvingoujAtDevolution
Copy link
Contributor Author

Unfortunately we can't change is_connect_failed as that would be a breaking change. So we have to keep the name the same there.

I apologize for my previous mistake. I would like to have your opinion on whether it is better to add is_err, mark is_connect_fail as deprecated, or simply keep using the same name but change the implementation.

I would argue that the first approach is better, as it aligns with the semantics it provides.

@notgull
Copy link
Member

notgull commented Feb 10, 2024

I apologize for my previous mistake. I would like to have your opinion on whether it is better to add is_err, mark is_connect_fail as deprecated, or simply keep using the same name but change the implementation.

I would argue that the first approach is better, as it aligns with the semantics it provides.

I'd argue that the first would be better.

@@ -1,36 +1,155 @@
use std::{io, net};
Copy link
Member

Choose a reason for hiding this comment

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

I would appreciate not changing this example and instead just wrapping the socket error bits in the configuration guards.

Copy link
Contributor Author

@irvingoujAtDevolution irvingoujAtDevolution Feb 22, 2024

Choose a reason for hiding this comment

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

can you elaborate what you mean by that?

Copy link
Member

Choose a reason for hiding this comment

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

Don't modify the Example too much; just make it so the is_err-specific code is guarded.

examples/tcp_client.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@irvingoujAtDevolution
Copy link
Contributor Author

please have a look again, I am not exactly sure if this is how you want it.

Copy link
Member

@notgull notgull left a comment

Choose a reason for hiding this comment

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

Overall LGTM, just some minor considerations regarding docs.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@notgull notgull left a comment

Choose a reason for hiding this comment

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

LGTM aside from one minor nitpick

src/lib.rs Outdated Show resolved Hide resolved
@irvingoujAtDevolution
Copy link
Contributor Author

😢 , @notgull could you please help me look at the wine CI thing? I have no idea how that works

@notgull
Copy link
Member

notgull commented Mar 21, 2024

Looks like a spurious error.

Copy link
Member

@notgull notgull left a comment

Choose a reason for hiding this comment

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

Thanks!

@notgull notgull merged commit e25b3b4 into smol-rs:master Mar 21, 2024
24 checks passed
@notgull notgull mentioned this pull request Mar 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants