[SERVER-53500] Investigate making nested futures equivalent to chained continuations Created: 23/Dec/20 Updated: 06/Dec/22 |
|
| Status: | Backlog |
| Project: | Core Server |
| Component/s: | Internal Code |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Task | Priority: | Major - P3 |
| Reporter: | Max Hirschhorn | Assignee: | Backlog - Service Architecture |
| Resolution: | Unresolved | Votes: | 0 |
| Labels: | sa-remove-fv-backlog-22 | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||
| Assigned Teams: |
Service Arch
|
||||||||||||
| Participants: | |||||||||||||
| Story Points: | 7 | ||||||||||||
| Description |
|
It is somewhat easy to misuse mongo::Futures by having them get chained through nesting (e.g. a future returned by another function) rather than chained at the top-level. Nesting futures can lead to hitting the kMaxDepth invariant (--dbg=on only) and it isn't immediately obvious to consumers of futures why a different syntax could have significantly different behavior.
Acceptance criteria:
|
| Comments |
| Comment by Billy Donahue [ 31/Jan/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
I wrote a more basic repro here. https://github.com/mongodb/mongo/compare/master...BillyDonahue:SERVER-53500_repro This doesn't involve any classes other than Future<void>, and is not multithreaded, so it should be simpler to diagnose. | |||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Billy Donahue [ 30/Jan/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
What is the purpose of the patch in the description? I applied it and didn't see any test failures. ... Update: For the record, on Mac at least, I needed to have --dbg=on AND --opt=on to repro this. Without --opt=on it doesn't fail. | |||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Matthew Saltz (Inactive) [ 28/Jan/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
Also after discussion with others, I don't think it's possible to make them equivalent exactly, since they aren't totally equivalent. For instance, you could have branching that allows you to decide which of two nested future chains to use. We should consider the Acceptance Criteria here more indicative of what needs to be done that the ticket title. | |||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Matthew Saltz (Inactive) [ 28/Jan/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
FWIW this invariant popped up again in a recent ticket. I think we need to prioritize this. I'm concerned that there may be edge cases that could cause serious problems in production that will be even harder to diagnose because that invariant won't be enabled. | |||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Max Hirschhorn [ 04/Jan/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
The different repro steps mentioned on this ticket passed locally for me with maxThreads=1. | |||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Matthew Saltz (Inactive) [ 04/Jan/21 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
Thanks for all the detailed repros! By the way - do you know if it's required to have more than one thread for the repro to work? If you don't know off the top of your head don't worry about going to try it - whoever ends up looking at this ticket can give it a shot. | |||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Max Hirschhorn [ 30/Dec/20 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
TryUntilLoopWithDelay chains the recursive call to run() onto the future returned by sleepFor() and so it can also hit the kMaxDepth invariant when the delay is short enough.
| |||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Max Hirschhorn [ 23/Dec/20 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
Note that with the current version of future_util.h, nesting AsyncTrys will also trigger the kMaxDepth invariant.
|