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

Use async-std in multipart example and treat all errors #228

Merged
merged 1 commit into from Jan 23, 2020

Conversation

torfmaster
Copy link
Contributor

Summary

In this PR I use async_std instead of blocking. Moreover, I treat all errors. I hope this makes the code more streamlined.

@JohnTitor
Copy link
Member

Test has been fixed on master, could you drop the commit?

@torfmaster
Copy link
Contributor Author

Fixed.

JohnTitor
JohnTitor previously approved these changes Jan 12, 2020
Copy link
Member

@JohnTitor JohnTitor left a comment

Choose a reason for hiding this comment

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

Overall looks good with one minor nit. It's nice that we can remove block part :)

multipart/src/main.rs Outdated Show resolved Hide resolved
@fafhrd91
Copy link
Member

let's create new example for async-std

@JohnTitor
Copy link
Member

Ah, okay, @torfmaster could you create a new example as @fafhrd91 mentioned?

@JohnTitor JohnTitor dismissed their stale review January 12, 2020 15:28

needs a new example

@torfmaster
Copy link
Contributor Author

Okay, I created a new example.

@JohnTitor JohnTitor merged commit 469f8b5 into actix:master Jan 23, 2020
@JohnTitor
Copy link
Member

Thanks!

@cdbattags
Copy link
Member

I'm curious how the event loop is handled with this under the hood.

Using async_std in tandem with tokio means multiple runtimes, no?

@cdbattags
Copy link
Member

@actix/contributors, can any of y'all shed some light on how this works under the hood?

I'm curious how the event loop is handled with this under the hood.

Using async_std in tandem with tokio means multiple runtimes, no?

@Dowwie
Copy link
Contributor

Dowwie commented Jan 29, 2020

@cdbattags please reserve the callout to all contributors for matters of importance

@cdbattags
Copy link
Member

@Dowwie, uhh, what does "matters of importance" mean here?

If people use async_std like this they'll need to know that it's going to use a completely separate runtime than "app"/tokio which is quite disconcerting.

@cdbattags
Copy link
Member

This is why @JohnTitor put out a blast to us 3 days ago about potentially trimming the team.

@jwdeitch
Copy link
Member

@cdbattags - sorry, nieve question, is the tokio runtime running in the context of this example?

@cdbattags
Copy link
Member

@jwdeitch actix-web uses tokio for runtime under the hood and async_std uses a different runtime. By calling an async_std method it will create yet another event loop on the same master thread, I believe, but I want to discuss in the context of this change.

@tyranron
Copy link
Member

@cdbattags @jwdeitch

I'm curious how the event loop is handled with this under the hood.

sorry, nieve question, is the tokio runtime running in the context of this example?

Well, tokio runtime is definitely running here, because actix_rt uses it under the hood, so thw whole actix-web server runs in actix_rt::main as usual.

Now, how async_std work here? Let's look a bit into sources...
For example, async_std::fs::create_dir runs async_std::spawn_blocking under-the-hood. So, technically, it starts the whole async_std runtime in separate threads, spawns the future there moving it into async_std executor and once it finished returns the result to the current thread.

@tyranron
Copy link
Member

From the documentation of async_std::task::spawn_blocking it seems to work near the same way as actix_threadpool::run does.

@cdbattags
Copy link
Member

My concern is that if we're showing this as a certified example then the architecture of the whole setup changes.

Where would we like to document this kind of stuff?

@cdbattags
Copy link
Member

Ideally, all futures spawned by usages of async_std won't hang around too long but a single usage of async_std will forever leave around this extra runtime without any sort of cleanup.

@Dowwie
Copy link
Contributor

Dowwie commented Jan 29, 2020

I concur with @cdbattags that this example is problematic. actix/examples is an official repo showing acceptable patterns, not anti-patterns. With that given, @cdbattags are you going to submit a PR to fix? Alternatively, it could be enhanced and documented to explain why this setup is problematic. "See this? Don't do this. Here's why."

@tyranron
Copy link
Member

@cdbattags

will forever leave around this extra runtime without any sort of cleanup.

Yup, the same happens with actix-threadpool.

@cdbattags
Copy link
Member

I'd say this isn't necessarily an "anti-pattern" but there's no great solution yet to mixing async_std and tokio as of yet.

Someday soon if the traits start lining up then it won't be a concern but for now we need to advertise the weirdness of this under the hood.

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

7 participants