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 support for user-supplied executors #3091

Merged
merged 10 commits into from
Oct 16, 2024

Conversation

stefnotch
Copy link
Contributor

This lets the user supply their own async executor. This should cover advanced use-cases, such as letting the user support the async_std crate.

@benwis
Copy link
Contributor

benwis commented Oct 10, 2024

@stefnotch It looks like you have three PRs, #3989 #3990 and this one, that overlap and add mostly same code. Is that correct? This one adds user supplied executors, which seems to be unique. Would I want to merge this one and close the other two?

@stefnotch
Copy link
Contributor Author

stefnotch commented Oct 10, 2024

@benwis Yes indeed. I first fixed the issue with an existing executor, and then used the slightly bigger API surface to support custom executors.

So this PR builds on top of the code of #3089 .

I figured I'd submit that as separate PRs, so that if this has a design issue, then it's at least easy to just merge #3089 .

Regarding #3090 , feel free to just close it. It's significantly less important to have support for it if there's already an easy way for users to supply their own executor.

(So yes, merging this and closing the other two would be perfect.)

@benwis
Copy link
Contributor

benwis commented Oct 10, 2024

@benwis Yes indeed. I first fixed the issue with an existing executor, and then used the slightly bigger API surface to support custom executors.

So this PR builds on top of the code of #3089 .

I figured I'd submit that as separate PRs, so that if this has a design issue, then it's at least easy to just merge #3089 .

Regarding #3090 , feel free to just close it. It's significantly less important to have support for it if there's already an easy way for users to supply their own executor.

(So yes, merging this and closing the other two would be perfect.)

Looks like the CI is complaining about formatting. Can you run it through cargo fmt?

@stefnotch
Copy link
Contributor Author

Ah, I see, a nightly Rust toolchain was required to get the desired formatting output. Just as documented in the contributing.md file. Done :)

@stefnotch
Copy link
Contributor Author

The CI is still failing. :( And while I fully understand what went wrong, I have no idea how to fix it.

What went wrong?
Executor::init_futures_executor() may only be called once. If an executor has already been initialized, it panics.
It also initializes a OnceLock static SPAWN: OnceLock<fn(PinnedFuture<()>)> = OnceLock::new();.
Those are shared across Rust unit tests, because all tests run in the same process.

I'm not sure how to go about changing the code to make it testable.

@raskyld
Copy link
Contributor

raskyld commented Oct 14, 2024

Hello!

I am pretty much interested by your PR since I need in #3063, a way to provide a Custom runtime for the wasm32-wasip2 target. Thanks for your work :) 🙏

I think you can solve the issue by adding a const https://doc.rust-lang.org/std/sync/struct.Once.html in your mod tests,
then, you could add an helper function that use https://doc.rust-lang.org/std/sync/struct.Once.html#method.call_once to call the init function only once and invoke this helper in the first lines of all of your tests.

@stefnotch
Copy link
Contributor Author

stefnotch commented Oct 14, 2024

@raskyld That would work if I had one type of executor. With the custom executors part, I now have two separate executors that I have tests for

  • Executor::init_futures_executor()
  • Executor::init_custom_executor(CustomFutureExecutor)

I also considered writing a custom executor for the unit tests that lets me swap out the underlying executor, but then the question arises: How do I test that? It's a bit of nontrivial code after all.

@stefnotch
Copy link
Contributor Author

stefnotch commented Oct 14, 2024

I suppose another option would be to run the any_spawner CI with two different feature flags. One to enable the custom executor tests, and another one to enable the futures executor tests.
@benwis Is there an easy way of doing that?

Edit: Nevermind, I read some more Rust documentation and figured out what the solution should be.

@stefnotch
Copy link
Contributor Author

Figured it out! I just had to use integration tests instead of unit tests.
Apparently if one puts their tests into a separate directory, then they get run in a separate process. Which solves the issue above 🎉

@stefnotch
Copy link
Contributor Author

Question regarding the CI: Why does CI (any_spawner) run cargo fmt -- --check? I would have expected that to happen in a separate CI job that checks the entire repository, instead of every single CI job re-checking the formatting.

@raskyld
Copy link
Contributor

raskyld commented Oct 14, 2024

Yeah reading the code I now see the problem, we need the tests to run as two separated process.
I don't know of any way to achieve that expect running two test command (which is kind of what you propose with the feature flag)

@raskyld
Copy link
Contributor

raskyld commented Oct 14, 2024

Just saw your comments, for some reason my GH page didn't get updated 🤣

Alright, swapping to integration tests make each module its own binary right?

@stefnotch
Copy link
Contributor Author

Alright, swapping to integration tests make each module its own binary right?

Yes, exactly. Rust by example confirms this

Each Rust source file in the tests directory is compiled as a separate crate.
https://doc.rust-lang.org/rust-by-example/testing/integration_testing.html

@benwis
Copy link
Contributor

benwis commented Oct 14, 2024

@stefnotch got another conflict for ya

@stefnotch
Copy link
Contributor Author

@benwis Fixed that for you :)

@benwis benwis merged commit ee66f6c into leptos-rs:main Oct 16, 2024
73 checks passed
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