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
Shared can interact badly with futures that don't always poll their subfutures #330
Comments
My current inclination is to say that I propose that the following invariant must be upheld by any valid implementation of I have changed heart on this topic since last week, largely because some of my code in capnp-rpc-rust now depends on the above invariant. |
This is one of the complications that we are facing because of the current asynchronous model of futures-rs (which will probably won't happen in callbacks model). I cannot see how it could be enforced by the type system, so if that proposal will be accepted, it has to be well documented. |
@alexcrichton I've updated my code above so that it's now a complete program that you can compile and run to see the behavior I'm worried about.
|
And to be clear, I think |
Ah ok thanks for the clarification! The title also makes more sense now in retrospect. So put another way, the way In your example the moded future is holding onto a I do agree that I'd prefer We could also perhaps consider this a bug in cc @aturon, this is a very interesting case where a lack of guarantees around what an unpark translates to can cause things to run awry. |
@alexcrichton This is indeed interesting. I'll note that it's basically the exact same issue we hit with mutexes for futures -- and if we followed @dwrensha's guarantee, it'd solve that problem as well. This is definitely food for thought in 0.2. |
Labeling as 0.2 just to make sure it's part of the general discussion... |
I'm curious whether there exist any non-contrived examples of futures which don't maintain that guarantee. |
Select is one example.
…Sent from my phone
On Jan 6, 2017, at 5:11 PM, David Renshaw ***@***.***> wrote:
I'm curious whether there exist any non-contrived examples of futures which don't maintain that guarantee.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
My gut reaction is that I am pretty against this guarantee. It just doesn't make sense in any non-trivial implementation of a future (of which tokio-proto is full of). It basically means that every call of poll you have to call poll on every sub future every time.. that doesn't seem reasonable. |
Interesting! Sounds like we need to adjust the property. How about the following? A future This should at least cover |
|
There are a number of problems w/ changing the behavior of futures in this direction. First, it either requires always polling all futures or allocating by using A worse outcome is that it forces a top level future to call poll on a sub future even if it isn't ready to handle the value returned by the sub future. This means that the top level future now needs to add additional logic to manage temporarily storing the value until it is ready to process it. Even worse, is that it is a huge wrench in the back pressure story. Imagine the sub future is a stream... now the top level future is required to call poll on it, potentially receiving an unbounded stream of values that are ready and having to buffer them. This sounds similar to the mutex problem, and I believe that the correct strategy is to look into adding a hook in the task system to enable "mutex" like futures to notify a task and then get signaled when the notified task got polled. This would enable a "mutex" to grant access to a task for a single poll even if the task in question doesn't touch the mutex during that poll. Of course, this would be a non trivial change and should be punted to 0.2. |
That being said, IMO the best 0.1 strategy is to use message passing based coordination. |
Ah, I had only considered |
This idea sounds promising. I've gone ahead and tried it out in my implementation of One somewhat sad part is that it requires a |
@dwrensha yeah the futures library is not currently optimized for heavy usage of Also as a side note I'd love to add single-threaded versions to the |
My current thinking is that it would not work out well to require that all impls of It helps to consider the case of streams. If we require futures to be faithful, then we probably also need to require streams to be faithful, because it's easy to convert back and forth between streams and futures. But what would that mean for The only plausible way I can see around such problems would be to make heavy use of The alternative is simply to not require faithfulness, and instead fix the bug in |
Actually, I'm not so sure that |
@dwrensha would you be interested in prototyping a version of |
I believe that I'm still seeing bad behaviour here as of 7470060. Notably, Periodically, for a It seems like what needs to happen is actually a recursive unpark/marking of the transitive dependents of the completing |
@stuhood could you post some code that demonstrates the problem? |
@dwrensha : I will try to get a repro this week. I have trace logs from the failure to wake up, and while they don't point directly to the culprit, I have a hypothesis. The |
Sorry guys, thought I'd have time to work on a repro during my vacation, but have been too busy. Next week. |
I still haven't managed to reproduce this outside of our repo, but I did find a much smaller workaround that might point to the cause. When our main thread executes (approximately) the following:
it will miss a wakeup about one in ten times (afaict from thread stacks, it is not a deadlock: all threads are On the other hand, the following succeeds reliably:
|
Any way for us to reproduce the problem would be helpful. I guess that by "repo" you are referring to some not-publicly-available components that are necessary to trigger the problem? |
(I'm opening this issue to continue the discussion from #305.)
The problem is that
Shared
, as it is currently designed, can interact poorly with certain futures, such as theModedFuture
sketched below. We should either formalize some reason whyModedFuture
is an invalid implementation ofFuture
, or we should redesignShared
to accommodate this kind of situation.The text was updated successfully, but these errors were encountered: