Fix the stack overflow in the compat layers (#1283)

[andreasots]

Since the LocalWaker is dropped immediately after polling the task01::Task can be stored on the stack.

Fixes #1283.

[Arnavion]

Maybe also fix and uncomment the doctests in futures-util/src/compat/tokio.rs so that this doesn't happen again.

[seanmonstar]

So this brings us back to cloning the Task on every single poll, even if the future doesn't need to clone it. Do we fully understand why the stack overflow is happening?

[cramertj]

@Nemo157 said in #1283

The issue appears to be because CurrentRef delays getting the task until it's needed it waits too long and the task's waker has already been replaced with a NotifyWaker that tries to use the CurrentRef, making a loop. Removing CurrentRef and producing a CurrentOwned straight away when needed fixes it. I assume CurentRef is there because CurrentOwned can be expensive with the Arc and accessing task::current, maybe there's an alternative optimization that could be done and will be safe against task::current being changed under it?

[cramertj]

@seanmonstar This is probably a single atomic op, right? In comparison, the UnsafeWake::clone_raw implementation requires actually allocating a new Arc-- that seems far more expensive to me, so I wouldn't think this to be that big of a deal. Do you disagree?

[cramertj]

If not, I'd prefer to land this and issue a release to get things working again, then iterate towards a better (but still correct) solution.

[seanmonstar]

I'm good with getting a fix first. It'd help if the fix includes a test that was failing, so new attempts at optimizing won't regress again.

[cramertj]

This change includes a test here -- does that work for you?

[Nemo157]

From the example and stacktrace in the linked issue the following happens:

1. tokio::run calls into <futures::task_impl::Spawn<T>>::enter which sets up the Task in TLS
2. futures_util::compat::compat03to01::<impl futures::future::Future for futures_util::compat::compat::Compat<Fut>>::poll creates a CurrentRef, then calls the wrapped future (the async block) with it as the LocalWaker in the context
3. the std libraries async layers are passed through (probably shouldn’t have fully snipped those)
4. futures_util::compat::compat01to03::<impl core::future::future::Future for futures_util::compat::compat::Compat<Fut>>::poll takes a reference to the current LocalWaker on the context and updates the Notify on the task to be this wrapped reference as WakerToHandle
5. <tokio_timer::delay::Delay as futures::future::Future>::poll calls futures::task_impl::current, this attempts to acquire an owned handle to the current Notify (again I did some overzealous snipping, see the linked gist in the issue for the full stacktrace)
6. to create the owned handle (NotifyWaker) futures_util::compat::compat01to03::<impl core::convert::From<futures_util::compat::compat01to03::WakerToHandle<'a>> for futures::task_impl::NotifyHandle>::from attempts to clone the LocalWaker it has
7. to clone the waker (into a CurrentOwned) <futures_util::compat::compat03to01::CurrentRef as core::task::wake::UnsafeWake>::clone_raw calls futures::task_impl::current to get the current task
8. goto 6

[seanmonstar]

Yep, that's great!

[cramertj]

Thanks so much for the fix! I'll get a release out today.