Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

refactor spawn API #14034

Closed
wants to merge 1 commit into from
Closed

refactor spawn API #14034

wants to merge 1 commit into from

Conversation

niklasad1
Copy link
Member

@niklasad1 niklasad1 commented Apr 27, 2023

I would want the spawn and spawn_blocking to return a spawn handle or something equivalent and as substrate uses tokio quite extensively looks reasonable to return tokio::task::JoinHandle instead.

Needed by #13992

No real code changes rather than tests and benchmarks

@niklasad1 niklasad1 added A3-in_progress Pull request is in progress. No review needed at this stage. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Apr 27, 2023
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2744791

@bkchr
Copy link
Member

bkchr commented Apr 27, 2023

A spawn handle is just:

let (rx, tx) = oneshot::channel();

spawner.spawn(async move { do_something.await(); let _ = tx.send(()); });

rx.await();

@niklasad1
Copy link
Member Author

yeah, but I didn't to make code so verbose with oneshot's when tokio spawn already has it..

I can add the oneshot stuff and you can complain in my other pr :D

@bkchr
Copy link
Member

bkchr commented Apr 27, 2023

I can add the oneshot stuff and you can complain in my other pr :D

Let's try that for now :P

yeah, but I didn't to make code so verbose with oneshot's when tokio spawn already has it..

If we add the JoinHandle, I would still make it generic. Aka something that wraps a Box<Future>. I know that we are currently using Tokio, but it doesn't leak that much to the internals right now.

@niklasad1
Copy link
Member Author

niklasad1 commented Apr 27, 2023

Fair enough

We could add some such as impl Future<Item = Result<(), ()>>` as return handle but yeah closing this

@niklasad1 niklasad1 closed this Apr 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A3-in_progress Pull request is in progress. No review needed at this stage. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants