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

Add Interest::PRIORITY #1647

Merged
merged 1 commit into from
Feb 14, 2023
Merged

Conversation

Thomasdezeeuw
Copy link
Collaborator

To trigger Event::is_priority on Linux and Android.

In the future we might want to include EV_OOBAND for kqueue, but that seems to be Apple only (FreeBSD and OpenBSD don't implement it at least).

Closes #1645

Does this solve the problem @poeschel, @Urist-McGit?

@poeschel
Copy link
Contributor

Since this is pretty much, what I proposed here: #1645 (comment)
I do not understand why you try to sell this as your work, but okay. Main thing is we solve the problem.

Does this solve the problem @poeschel, @Urist-McGit?

Unfortunately no, it does not solve it fully. You missed debug-fmt-ing the priority interest. In your case debug printing a priority interest alone does panic. Have a look at my proposal in src/interest.rs lines 186-192.

Thank you very much for your work regarding this issue.

@Thomasdezeeuw
Copy link
Collaborator Author

Thomasdezeeuw commented Feb 11, 2023

Since this is pretty much, what I proposed here: #1645 (comment) I do not understand why you try to sell this as your work, but okay. Main thing is we solve the problem.

I only saw the example, which had a bug I pointed out, then I got a pr (#1646) with the exact same bug and an unwanted dependency. So, in my free time I tried fix the problem you were having...

If you had the solution and a working fix, why not submit that in as a pr?

@poeschel
Copy link
Contributor

I only saw the example, which had a bug I pointed out, then I got a pr (#1646) with the exact same bug and an unwanted dependency. So, in my free time I tried fix the problem you were having...

Don't get me wrong. As I said already: I am very thankful for your work. Be it:

  • looking and commenting on my issue
  • looking and commenting on the example we provided
  • looking at the PR with the test demonstrating the problem
  • your PR here
  • your work on mio in general

I am sorry, that you did not see my proposed fix hidden in the comment on the issue. This is my fault. I thought you get notified, if you were commenting on an issue and I add a new comment.

If you had the solution and a working fix, why not submit that in as a pr?

This is because I am really unsure. You are the holy magicans that make this work and keep the code tidy andclean and I am a clueless guy having an issue. At first I was unsure if the problem I saw is really an issue in mio or if it is just me doing something wrong. While investigating the issue, I saw in the history that there actually has been some PRIORITY stuff already, but it was removed. Maybe for a good reason? Before submitting a PR I wanted to know where the problem really is. During our discussion on the issue, it became clear to me, that there really is an issue and I started thinking about how a solution could look like. This then became the code in my proposal.

@poeschel
Copy link
Contributor

Could you please add something like
https://github.com/poeschel/mio/blob/282edd32c7459d14cd4efc0ecafd652ab806f30f/src/interest.rs#L186-L192
? Maybe you also need #[cfg(any(target_os = "linux", target_os = "android"))] ?
I think then your PR is fine.

@Thomasdezeeuw
Copy link
Collaborator Author

Don't get me wrong. As I said already: I am very thankful for your work. Be it:

Thank you.

This is because I am really unsure.

That's what pr reviews are for, it's an easy place to see what you're proposing and it's a place for a discussion.

You are the holy magicans that make this work and keep the code tidy andclean and I am a clueless guy having an issue. At first I was unsure if the problem I saw is really an issue in mio or if it is just me doing something wrong. While investigating the issue, I saw in the history that there actually has been some PRIORITY stuff already, but it was removed. Maybe for a good reason? Before submitting a PR I wanted to know where the problem really is. During our discussion on the issue, it became clear to me, that there really is an issue and I started thinking about how a solution could look like. This then became the code in my proposal.

No magicians at work, we're all writing the same code.

I'll push a commit with the formatting fix and give you co-author credit.

Copy link
Contributor

@poeschel poeschel left a comment

Choose a reason for hiding this comment

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

The code looks good to me now.
But what happened to the Co-authored-by?

Thank you!

@Thomasdezeeuw
Copy link
Collaborator Author

@poeschel it seems you commit with a different email address than GitHub recognises, what do you prefer I use?

To trigger Event::is_priority on Linux and Android.

In the future we might want to include EV_OOBAND for kqueue, but that
seems to be Apple only (FreeBSD and OpenBSD don't implement it at
least).

Co-authored-by: Lars Pöschel <[email protected]>
@poeschel
Copy link
Contributor

@poeschel it seems you commit with a different email address than GitHub recognises, what do you prefer I use?

Best is, you use [email protected] I also added this to my github profile.
Thank you!

@Thomasdezeeuw Thomasdezeeuw merged commit ae9fb76 into tokio-rs:master Feb 14, 2023
@Thomasdezeeuw Thomasdezeeuw deleted the prio-interest branch February 14, 2023 10:47
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.

A way to add EPOLLPRI event type to the epoll selector
3 participants