Wake impl for Waker should hold a weak reference

[carllerche]

Looking at the implementation of Wake for ArcWrapped, it looks like it holds a strong reference of the Wake instance.

Since the Wake instance is usually the task, containing the future, and all associated state, it seems like wakers should hold a weak reference.

There could be an arbitrary number of wake handles around for arbitrary amounts of time. Holding a strong reference will prevent the task state from being cleared.

[aturon]

It's critical to hold a strong reference, because at least some executors don't keep a handle to blocked tasks; the fact that you have Wakers floating around is what keeps the task alive, knowing that it could be restored to the executor when the right event occurs.

FWIW, nothing here has changed from futures 0.1.

[carllerche]

Alright, it isn't critical as I can just use the unsafe trait. I would just say that this behavior does not make it possible to implement a clean shutdown for an executor (as the tasks cannot be dropped).

[aturon]

@carllerche Do you understand the issue I'm raising though? I don't think it's an option to change this.

Also, why not just keep the underlying future in an Option and drop that on shutdown? That's what the Tokio threadpool does AFAIK.

[carllerche]

@aturon I understand the issue.

In my opinion, if one relies on this behavior, it implies the executor does not correctly implement a shutdown process.

Either way, I can rely on the unsafe trait.

[aturon]

@carllerche To clarify: I think you're saying that an executor that wants to implement shutdown needs to keep a the single strong reference to all tasks ever spawned.

But doesn't that create potential for leakage? If a task is blocked, and all Wakers for it have been dropped, AFAICT the task wouldn't be dropped until shutdown.

[carllerche]

@aturon It depends on the executor's impl detail, but I plan on signalling the executor which then performs cleanup.

Also, all wakers dropping before the future completing is indicative of a bug.

[aturon]

Also, all wakers dropping before thee future completing is indicative of a bug.

Ahha, you're right -- I was thinking about this from the future side, not the task side. Makes sense.

Ok so with all of that, I think it may indeed be worth changing this. @cramertj any thoughts?

[cramertj]

@aturon It breaks futures-cpupool and it would break my current fuchsia executor, but the change doesn't seem unreasonable and the fix is fairly easy.

For the sake of clarification: right now wake takes an &Arc<Self> and pushes itself onto a queue when it is ready. This would still happen, but it would first Waker would internally attempt an upgrade, which would only succeed if you'd stored a strong reference somewhere (so now my executor and futures-cpupool need to hold a (mutex-protected) slab of strong references, which will get removed when the corresponding task is completed). Does this sound right to both of you?

Or would Wake take Weak<Self> instead?

There could be an arbitrary number of wake handles around for arbitrary amounts of time. Holding a strong reference will prevent the task state from being cleared.

The Future stored in the task is an Option, so it and any allocations or references it holds are cleared upon completion. The task type that wraps it isn't freed until all of its potential Wakers go away, though (note that even with Weak, you have to leave around the section of memory that stores the current count so that upgrades can be attempted).

[carllerche]

@cramertj yes, I know that when the future is completed, it is dropped.

My point is, with the thread pool as it is implemented today, when the pool is dropped the workers shutdown, but all spawned futures are not touched, so they can stay around for arbitrary amounts of time.

[stbuehler]

Keeping weak references in ArcWrapped (and probably ArcNode in futures_unordered?) means you need to upgrade on every wake (i.e. additional atomic ops). I think another solution would be making a Wake implementation not keeping the task alive.

[cramertj]

@carllerche

all spawned futures are not touched, so they can stay around for arbitrary amounts of time.

Yup, if you're creating and deleting pools, this will leak tasks. I guess it's a matter of deciding what is more important: the extra atomic ops, or consistent cleanup. Note that you could implement a system for opting into cleanup by storing an Arc<MyWeak<T>> (struct MyWeak<T>(Weak<T>);). I think the real issue here is deciding what the default should be. Personally, I think it will be uncommon to delete pools multiple times within the course of an application, and it seems fine to provide this via opt-in behavior that comes with a perf cost, rather than pessimizing all other applications (via the upgrade calls).

I think I don't care too much either way here, so if you or @aturon have a strong preference I'm fine going w/ that.

[carllerche]

@cramertj

I think it will be uncommon to delete pools multiple times within the course of an application

Yes, but IMO that doesn't mean that the provided ThreadPool should leak in those cases.

Either way, as I mentioned earlier, you can achieve either option by using UnsafeWake. This issue asks:

• Should the default behavior be a weak reference
• Should the provided thread pool implementation leak tasks when it is dropped.

[cramertj]

@carllerche Yup, I agree with your summary. i'm fine with either behavior.

[aturon]

Thanks all. After this thread, I feel pretty comfortable with the defaults we've chosen, rather than imposing the upgrade costs across the board. I'm going to close this out.

[seanmonstar]

Just a thought, but it seems possibly better to default to safer (slash not leaking), and allow implementations who know better to opt-in to that with the unsafe trait.

It should be harder to write those bugs, not easier.