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 AsyncRuntime::oneshot #1025

Closed
wants to merge 2 commits into from
Closed

Conversation

Miaxos
Copy link
Contributor

@Miaxos Miaxos commented Feb 22, 2024

A draft for AsyncRuntime::oneshot, there is a clear cost associated to abstracting it this way, WDYT?

Checklist

  • Updated guide with pertinent info (may not always apply).
  • Squash down commits to one or two logical commits which clearly describe the work you've done.
  • Unittest is a friend:)

This change is Reviewable

Signed-off-by: Anthony Griffon <anthony@griffon.one>
Signed-off-by: Anthony Griffon <anthony@griffon.one>
@Miaxos Miaxos closed this Feb 22, 2024
@drmingdrmer
Copy link
Member

This PR seems to be generally fine. What's the reason closing it?

@schreter
It comes to my mind that oneshot or other sync primitives may not have to be part of the AsyncRuntime.
Because AFAIK, they do not rely on the runtime functionality to work, not like sleep or IO methods.

make sense?

@schreter
Copy link
Collaborator

It comes to my mind that oneshot or other sync primitives may not have to be part of the AsyncRuntime.

Well, actually I'd like to have them part of the AsyncRuntime :-). Although it's not strictly necessary and typically they work with other runtimes, they may not always. For example, tokio counts number of times the task polled the primitive and deliberately returns Pending also if the primitive is ready to decrease latency. Other runtimes have similar approaches. However, these work only if they are running on "their" runtime. So it's desirable to use the "correct" primitive.

(and plus, we have issues with malfunction safety with Tokio primitives, so we replaced them - unfortunately, upstream there is not a big chance to get changes in this area accepted, since it would require changing some core concepts)

@Miaxos
Copy link
Contributor Author

Miaxos commented Feb 23, 2024

I used specialization for this PR to have the Debug implementation on OneshotReceiver<T> (and we want it) and without needing T: Debug (or it cause a lot of issues as T would need to be Sized): https://github.com/datafuselabs/openraft/pull/1025/files#diff-0ba3d3f3eaba6e353cc2461758c777b314eb08b0c9cd1812ac06f71d1ab39b84R189-R199

As specialization is needed, it won't compile on stable anymore, so I need to find another way to do it without specialization, mb something like having a generic on AsyncRuntime directly to define the generic for the OneshotReceiver.

EDIT: It seems I was just too tired yesterday, I can do it without specialization easily, sorry about that, will re-open it with fixed tests so we can discuss it

-> #1026

Copy link
Collaborator

@schreter schreter left a comment

Choose a reason for hiding this comment

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

When the non-generic code uses a concrete type, the specialization is not needed. You can also sort-of specialize using autoref specialization, see for instance here. We used it at couple of places in our project to do exactly that - call a different impl based on whether Debug is supported or not (or another trait). Unfortunately, it doesn't work well in generic context (inside of a method taking T you cannot autoref-specialize on T: Debug, for instance).

I am also hoping that specialization will arrive soon - it is so much easier to do things with it.

Reviewable status: 0 of 22 files reviewed, all discussions resolved

@drmingdrmer
Copy link
Member

When the non-generic code uses a concrete type, the specialization is not needed. You can also sort-of specialize using autoref specialization, see for instance here.

Great! I've been looking for a method like this for a while!

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.

3 participants