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

switch to async-io + blocking + multitask #836

Merged
merged 6 commits into from Jul 26, 2020

Conversation

Keruspe
Copy link
Member

@Keruspe Keruspe commented Jul 17, 2020

smol now being divided in smaller crates, I tried porting async-std to those.

The async-io and blocking parts work fine and all tests seem to pass.

As of now, there are some tests hanging, namely because when we spawn a future from inside a block_on, it seems not to be polled, haven't figured out why yet.

The tokio enter function is directly copied from smol

@Keruspe
Copy link
Member Author

Keruspe commented Jul 18, 2020

All tests pass here now

@Keruspe
Copy link
Member Author

Keruspe commented Jul 18, 2020

Not sure what's wrong with ppc64 cross compilaion

@Keruspe
Copy link
Member Author

Keruspe commented Jul 18, 2020

#837 just confirmed that the ppc64 failure is unrelated, not sure about the macos one: (signal: 4, SIGILL: illegal instruction)

@ghost
Copy link

ghost commented Jul 18, 2020

@Keruspe To figure out the crash on macOS, open .github/workflows/ci.yml and add RUST_BACKTRACE: 1 to the env section.

@Keruspe
Copy link
Member Author

Keruspe commented Jul 18, 2020

Doesn't seem to give more info, but I think the ci files from master are used. I tried to temporary comment out ppc64 and the ppc64 tests were still run

@Keruspe
Copy link
Member Author

Keruspe commented Jul 19, 2020

I created another project to toy with the CI.

RUST_BACKTRACE=1 doesn't seem to have an impact of the macOS builder output.

The ubuntu and windows builders pass all the tests just fine. Apart from the ppc64, the other cross compilation targets pass fine too.

I'm not really familiar with debugging macos issues so I don't really know where to start

@Keruspe
Copy link
Member Author

Keruspe commented Jul 19, 2020

The first two commits work fine, the switch to multitask is the one causing the problem on macos

@ghost
Copy link

ghost commented Jul 19, 2020

Here are the errors I'm getting on my macOS machine: https://gist.github.com/stjepang/bbe3f07c947b2fd93a47b2e667f4428a

@Keruspe
Copy link
Member Author

Keruspe commented Jul 19, 2020

@stjepang with this last commit, tests pass for OSX.

Looks like async_io::parking::Parker calls Reactor::get() in its Drop impl, which fails on OSX if no IO has been done in the current thread?

@Keruspe
Copy link
Member Author

Keruspe commented Jul 19, 2020

Ok, instead of using the parking crate, forcing initialization of the reactor by parking for 0ms seems to work too.

Not sure if there's a more elegant solution, something that we maybe want to fix on the async-io's side of things?

@Keruspe
Copy link
Member Author

Keruspe commented Jul 19, 2020

Just opened smol-rs/async-io#4 as an alternative workaround

@ghost
Copy link

ghost commented Jul 20, 2020

Thanks for the PR to async-io, this is now fixed in async-io v0.1.4.

@Keruspe
Copy link
Member Author

Keruspe commented Jul 20, 2020

Apart from the ppc64 cross compile which is broken even outside of this PR, everything should work fine now

@ghost
Copy link

ghost commented Jul 20, 2020

Looks good to me :) If all goes well, I think it’s time to stabilize crates async-io, blocking, and multitask.

@Keruspe
Copy link
Member Author

Keruspe commented Jul 20, 2020

Rebased on top of master

@ghost
Copy link

ghost commented Jul 20, 2020

I think you can now remove futures-timer from the [dependencies] section in Cargo.toml

@Keruspe
Copy link
Member Author

Keruspe commented Jul 20, 2020

It's still used if unstable is enabled and default disabled it seems?

@dignifiedquire
Copy link
Member

yes it is needed atm when there is no runtime at all imcluded

@ghost
Copy link

ghost commented Jul 20, 2020

Right, so we should be able to remove the dependency in the typical case. It pulls in a lot of transitive dependencies...

@dignifiedquire
Copy link
Member

@stjepang maybe the timers could live in their own crate? that way we could use them in the non runtime build, without pulling in anything else.

@ghost
Copy link

ghost commented Jul 21, 2020

@dignifiedquire Timers live in the async-io crate. We should exclude that dependency in the non runtime build.

@Keruspe
Copy link
Member Author

Keruspe commented Jul 21, 2020

I don't think there's a way to specify in Cargo.toml that a dep should be there only if an option is unset (wrt futures-timer).

It doesn't seem to bring that much deps when wasm-bindgen isn't enabled though?

@ghost
Copy link

ghost commented Jul 23, 2020

Is the nightly fixed now?

@Keruspe
Copy link
Member Author

Keruspe commented Jul 23, 2020

Should be

@Keruspe
Copy link
Member Author

Keruspe commented Jul 23, 2020

With the switch to async-executor, the code is simpler, we gain more integration with smol 0.3, but the caveat is that tasks spawned onto the multi-threaded global executor cannot run local tasks anymore (since async-executor can only run the multi-threaded executor or the local executor on a thread but not both, so the runtime threads now only runs the global multi-threaded one)

@Keruspe
Copy link
Member Author

Keruspe commented Jul 23, 2020

With something like smol-rs/async-executor#3 + reverting the src/rt/mod.rs change and dropping task::executor::run_global works too and restore the previous behaviour WRT local tasks

@ghost
Copy link

ghost commented Jul 23, 2020

What if we restrict spawn_local() as suggested here? #815 (comment)

That would simplify things because only block_on() would run a LocalExecutor, and spawned threads would only run an Executor.

@Keruspe
Copy link
Member Author

Keruspe commented Jul 23, 2020

Then I guess that's more or less what the current port to async-executor does?

@Keruspe
Copy link
Member Author

Keruspe commented Jul 24, 2020

@stjepang What about returning an Option<Task<T>> from Task::local() instead of panicking? This leaves the choice to the caller to panic or handle the lack of local executor more "softly"

@ghost
Copy link

ghost commented Jul 24, 2020

I expect in 99% cases people will just do Task::local(future).unwrap(), having the same effect. That said, it would be good to have a way of checking "am I inside an executor?", but I don't think it should block this PR...

@Keruspe
Copy link
Member Author

Keruspe commented Jul 24, 2020

Anything else holding this PR back?

@dignifiedquire
Copy link
Member

Anything else holding this PR back?

  • seems CI is sad for some reason
  • the code looks good, but I want to give it a test run on a largish codebase I work on to make sure I don't see any surprises

@Keruspe
Copy link
Member Author

Keruspe commented Jul 24, 2020

CI is just windows probably never tested without default and with unstable.

and add a CI check for it

Fixes async-rs#842

Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com>
Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com>
Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com>
Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com>
Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com>
Signed-off-by: Marc-Antoine Perennou <Marc-Antoine@Perennou.com>
@Keruspe
Copy link
Member Author

Keruspe commented Jul 24, 2020

Squashed those fixes into one commit and rebased

Copy link
Member

@dignifiedquire dignifiedquire left a comment

Choose a reason for hiding this comment

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

lgtm, and seems to be working as before

@dignifiedquire dignifiedquire merged commit f9c8974 into async-rs:master Jul 26, 2020
@Byron
Copy link

Byron commented Jul 29, 2020

Something to be aware of in case you see issues related to Timers: smol-rs/async-io#6

@zhuxiujia
Copy link

smol now being divided in smaller crates

way smol being divided in smaller crates???

@jon-chuang
Copy link

Is there discussion on why smol / async-std was preferred to tokio for tremor? I'm trying to understand tremor as well as building some Rust async support for a cloud-native project. The info would be helpful. Thanks.

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.

None yet

5 participants