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

Copy illumos waker pipe work around to eventfd #1826

Merged
merged 2 commits into from
Aug 10, 2024
Merged

Conversation

Thomasdezeeuw
Copy link
Collaborator

It seems the same behaviour we seen for pipe waker implementation is also true for eventfd. Work around this by resetting the waker before writing to it.

It seems the same behaviour we seen for pipe waker implementation is
also true for eventfd. Work around this by resetting the waker before
writing to it.
Copy link

@sunshowers sunshowers left a comment

Choose a reason for hiding this comment

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

Thank you for your time working on this issue! While I continue to believe that switching back to pipes is the least risky way to move forward, I have a couple of comments if you choose to go down this route.

src/sys/unix/waker/eventfd.rs Outdated Show resolved Hide resolved
src/sys/unix/waker/pipe.rs Outdated Show resolved Hide resolved
@Thomasdezeeuw
Copy link
Collaborator Author

The CI will be fixed tomorrow, see #1827 (comment).

Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Assuming the Illumos folks will test this to sanity check it, LGTM. We may want tokio-rs/tokio#6763 in mio as well.

@Thomasdezeeuw
Copy link
Collaborator Author

Assuming the Illumos folks will test this to sanity check it, LGTM. We may want tokio-rs/tokio#6763 in mio as well.

I would like better CI support for illumos (or any OS that we currently don't test),

@Thomasdezeeuw
Copy link
Collaborator Author

CI should be fixed by #1828, so merging this.

@Thomasdezeeuw Thomasdezeeuw merged commit 38d1946 into master Aug 10, 2024
58 of 60 checks passed
@Thomasdezeeuw Thomasdezeeuw deleted the fix-illumos branch August 10, 2024 09:02
@jclulow
Copy link
Contributor

jclulow commented Aug 10, 2024

Assuming the Illumos folks will test this to sanity check it, LGTM. We may want tokio-rs/tokio#6763 in mio as well.

For the record:

  • This is expressly the opposite of what we asked for, and has been integrated over our strong objections
  • A cursory inspection and passing similarity is not sufficient at all to determine that this workaround and the existing workaround are in any way related
  • No amount of testing we could possibly perform in the next day with a few unit tests would produce the same risk profile as merely reverting the original regression

It's unfortunate that the members of the @tokio-rs/illumos maintenance group were ignored and bypassed here twice: once when the original change was made without notice or testing during a larger refactoring, then again here. We are strongly motivated to help as this software is critical for us and for others; please let us do so!

@Thomasdezeeuw
Copy link
Collaborator Author

@jclulow I didn't want to do this, but if we're adding to the record...

I really, really disliked communicating with you and @sunshowers. Every single comment the both of you made in #1824 read like a marketing blob. We're engineers, please be brief in your communication. No need to highlight every single first sentence, nor write paragraphs when the information can be conveyed in a single sentence.

I preferred to determine the cause of the bug, rather then ignore it. This didn't see to be too urgent for either of you. I understand that you rely on Tokio, and thus Mio, but it's fine to stick to a previous version for a couple of days longer, giving us time to fix the bug properly.

Once I did have time to look at the issue it took me whole of 2 minutes to figure out it was most likely the same issue as the one we already hit in the pipe waker implementation. Giving me another indication that neither of you really bother to look at the problem at hand (but I could be wrong here and this was simply not communicated to me).

Doubling down with comments such as #1824 (comment) and #1826 (comment) is really not appropriated by me. I didn't have to help, I could just say illumos is no longer supported. But I didn't and I won't. I always try to help out OSs that I personally don't run, but I do need help from the people who do run them. I failed that get from either of you.

In the future I hope situations like this are resolved differently.

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.

4 participants