Expose that BasicQueue expects blocking spawn
#5860
Conversation
Up to now `BasicQueue` expected a closure that to spawn a `Future`. This was expected to be a closure that spawns a blocking future. However, this wasn't documented anywhere. This pr introduces a new trait `SpawnBlocking` that exposes this requirement to the outside.
bkchr
added
A0-please_review
bkchr
requested review from
Demi-Marie,
NikVolf,
andresilva and
sorpaas
as code owners
| ); | ||
|
|
||
| spawner(future.boxed()); | ||
| spawner.spawn_blocking("basic-block-import-worker", future.boxed()); |
There was a problem hiding this comment.
@tomaka might need to update his grafana dashboard :)
primitives/core/src/traits.rs
Outdated
| } | ||
|
|
||
| /// Something that can spawn a blocking future. | ||
| pub trait SpawnBlocking: futures::task::Spawn { |
There was a problem hiding this comment.
| pub trait SpawnBlocking: futures::task::Spawn { | |
| pub trait SpawnBlocking { |
Let's not make it optional to pass a name.
|
When I see a new trait being introduced, I can't help but be annoyed. This adds a lot of complexity and contributes to the spaghetti-ification for not much. Basically all of our Futures are "blocking" because we call in the client. The import queue is just a worst case scenario in that it is probably does more database accesses than the others, but there's no fundamental distinction between a "blocking" and a "non-blocking" future. The If this really needs to be done, what about changing the closure to accept an additional |
|
I agree with @tomaka on this. |
|
The point why I come up with this, because I needed to port the changes to Cumulus. If I would just have read the documentation, I would probably just have spawned the future without using I don't understand how forwarding random closures can help to solve anything? Especially as this random closure directly sets an opioniated name for the future. What happens when I refactor the BasicQueue and need to spawn another future? Right, I would use the closure and spawn another future using the same name. This increases the spaghettification of the code. I would like to have used a upstream trait as well, but there is nothing. |
Up to now
BasicQueueexpected a closure that to spawn aFuture.This was expected to be a closure that spawns a blocking future.
However, this wasn't documented anywhere. This pr introduces a new trait
SpawnBlockingthat exposes this requirement to the outside.polkadot-companion: paritytech/polkadot#1061