Unset the needs-notify bit in a Notifier when a Future is fetched #1851
Conversation
|
Although I can't comment on the code changes, I have run our failing test using this patch at d61ebec and confirmed it resolves the issue we were seeing in our integration tests of |
TheBlueMatt
force-pushed
the
2022-11-fix-broken-futures-----again
branch
from
d61ebec to
d2e8a0c
Compare
| @@ -212,10 +215,11 @@ impl<'a> StdFuture for Future { | |||
| fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> { | |||
| let mut state = self.state.lock().unwrap(); | |||
| if state.complete { | |||
| state.callbacks_made = true; | |||
There was a problem hiding this comment.
Why are we doing this? A completed future does not necessarily mean callbacks were made. Removing this line also don't have any effect on the tests.
There was a problem hiding this comment.
Oops, sorry, failed to include that in the test. As far as we're concerned, the future polling Ready is calling the user code to persist. When we hit the waker, we tell the runtime (eg tokio) that its time to poll the future again. Once it does that (and we return Ready) that tells the runtime its time to do whatever was waiting on the future (probably user code to persist the ChannelManager.
|
Feel free to squash. |
This appears to have been added with the intent of having a sealed trait, which was never committed.
If a `Notifier` gets `notify()`ed and the a `Future` is fetched, even though the `Future` is marked completed from the start and the user may pass callbacks which are called, we'll never wipe the needs-notify bit in the `Notifier`. The solution is to keep track of the `FutureState` in the returned `Future` even though its `complete` from the start, adding a new flag in the `FutureState` which indicates callbacks have been made and checking that flag when waiting or returning a second `Future`.
When we return from one of the wait functions in `Notifier`, we should also ensure that the next `Future` doesn't start in the `complete` state, as we have already notified the user, as far as we're concerned. This is technically a regression from the previous commit, but as it is a logically separate change it is in its own commit.
When we mark a future as complete, if the user is using the `std::future::Future` impl to get notified, we shouldn't just assume we have completed the `Future` when we call the `Waker`. A `Future` may have been `drop`'d at that point (or may not be `poll`'d again) even though we wake the `Waker`. Because we now have a `callbacks_made` flag, we can fix this rather trivially, simply not setting the flag until the `Future` is `poll`'d `Complete`.
TheBlueMatt
force-pushed
the
2022-11-fix-broken-futures-----again
branch
from
f4e8686 to
0a1e48f
Compare
|
Squashed without further changes. |
Codecov ReportBase: 90.71% // Head: 90.71% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1851 +/- ##
=======================================
Coverage 90.71% 90.71%
=======================================
Files 89 89
Lines 47923 47952 +29
Branches 47923 47952 +29
=======================================
+ Hits 43473 43500 +27
- Misses 4450 4452 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
- `0.0.112` introduced a bug where calls to `get_persistable_update_future` always return a `Future` which is `Ready`. - After some back-and-forth with Matt, the bug was resolved in lightningdevkit/rust-lightning#1851 - Although 1851 hasn't been merged yet, I went ahead and picked the changes into the `lexe-v0.0.112-with-fix` branch of our LDK fork, and changed the workspace root `Cargo.toml`s to use this patch. - One extra perk is that we now have early access to the `ChannelReady` event which is set to be released with `0.0.113`. - Once `0.0.113` is released, we should switch back to using the crates.io version of LDK.
- `0.0.112` introduced a bug where calls to `get_persistable_update_future` always return a `Future` which is `Ready`. - After some back-and-forth with Matt, the bug was resolved in lightningdevkit/rust-lightning#1851 - Although 1851 hasn't been merged yet, I went ahead and picked the changes into the `lexe-v0.0.112-with-fix` branch of our LDK fork, and changed the workspace root `Cargo.toml`s to use this patch. - One extra perk is that we now have early access to the `ChannelReady` event which is set to be released with `0.0.113`. - Once `0.0.113` is released, we should switch back to using the crates.io version of LDK.
If a
Notifiergetsnotify()ed and the aFutureis fetched,even though the
Futureis marked completed from the start andthe user may pass callbacks which are called, we'll never wipe the
needs-notify bit in the
Notifier.This addresses (yet another) Future bug that a user caught :(. For the sake of completeness this also avoids considering a future as complete until we get
poll'd.