cannot recursively call into Core
vorner opened this issue · 12 comments
Hello
It seems that by using tokio-executor under the hood, the behaviour changed in backwards-incompatible way. If inside one future I create a new core and try to run something in it, it panics with cannot recursively call into Core (https://github.com/tokio-rs/tokio-core/blob/master/src/reactor/mod.rs#L242).
I know this is an edge case and that I'd try to avoid it in a real application (I use it in some tests, checking some edge cases of my own) and that the reason for it is to actually disallow this degenerate case, but it was allowed before, so I don't know if it's OK to forbid it now.
This behavior was, in theory, never permitted. It was just never enforced.
Could you explain more how you were using it? It would probably be best to figure out a way around it.
As I said, it's a test, this one: https://github.com/vorner/corona/blob/master/test/recursive_core.rs. I put some core-related things into TLS, and I wanted to check I don't mix it up if there are multiple and take turns or something. I can probably just throw the test out, when it comes to that. I'm just waiting for the waters to calm down before I port the whole library onto the new tokio anyway, so it won't be relevant any more.
My point was more about this might be an oversight or something like that. Anyway, if it acts like it should, it probably should be documented in the Panics section.
If I’m understanding this properly, doesn’t this meant that two independent libraries can’t create Cores separately in the case where one might depend on the next?
Especially in the case of wrappers that might be attempting to create synchronous interfaces?
Do we need to enforce that a Core is the only Core on any particular thread?
My understanding is you can run one core, let it terminate, then run another. But if you create one core, spawn a future into it and inside that future create another core and run it, it'll panic because you most likely don't want that. The inner core would be blocking inside the outer one and the outer one would not be able to execute futures at the time.
And in my understanding libraries should never create their own cores, but be provided with a handle (or tokio::runtime) by the application and spawn whatever they need onto that.
@bluejekyll as @vorner stated above, you can create an arbitrary number of cores per thread. What you cannot do is block on a core (call run) from within a future as that would be pretty disasterous to the Async runtime characteristics.
Is this safe?
https://github.com/bluejekyll/trust-dns/blob/master/resolver/src/resolver.rs#L126
Also, I know it’s not efficient, and I’ve planned to change it, but I’m concerned it’s not safe at the moment.
I'm hitting this issue with two libraries that manage their own cores (actix-web & ldap3). This is definitely a breaking change for me.
A few questions:
- Is this actually something that should be enforced by tokio? If there is recursive cores then the inner one will block yes, but it should still work fine otherwise (and has been working for me in the past).
- If it is enforced, should it panic? I would assume that it should propagate up as a Result?
- How would you manage this with two libraries that are trying to hide implementation details to support blocking use? Seems like this is very leaky if you need to now worry about whether another core is running or not.
Ah yes, that is unfortunate. However, nesting calls to Core::run is just not supported at this point. It never really was, it just was not enforced. The plan has always been to enforce it at some point.
I would rather dig in and figure out why you need to nest calls to Core::run, perhaps there is another way to go about doing it.
If there is recursive cores then the inner one will block yes, but it should still work fine otherwise [...]
Actually, there are several cases where it will grind the thread to a halt. I've seen this reported before in hyper, where people have a Core running their server, and wish to start a client to pipe a request body to another location. They end up with core.run(client.request(req_using_server_body)), and so, the inner Core will block waiting for the incoming server body to make progress, but it cannot since the thread is blocked by the inner Core. Panicking is probably better here.
I agree in my case it'd probably make sense to make the trait I was working with futures-aware, but that will take some time to refactor. I have basically just spawned a separate thread to work around it for the time being.
I still was initially horrified over the panic, as I was not expecting it to happen at all! It would be nice if there was a Result returned instead, so I can deal with this at compile time, not when it popped up during runtime. I understand that'd probably break the signature of run though, so may not be possible.
At the bare minimum, maybe some documentation along the lines of: Calling this method when inside an existing future's execution (i.e, recursively) on the same thread will cause a panic, as it would otherwise cause a deadlock
Yeah, we cannot break the signature of run. The current signature is unfortunate.
Would you mind submitting a PR that includes the snippet under the "panics" section of the docs?
Ok, I will submit a PR