Continue protecting against futurelock #119
Conversation
| // Out of an abundance of caution, to avoid futurelock: drop all | ||
| // possible unpolled futures before invoking terminate. | ||
| drop(rebalance_interval); | ||
| drop(backend_status_stream); | ||
| drop(resolver_stream); |
There was a problem hiding this comment.
I don't think this is necessary. The Interval and Streams are not Futures, they are values with methods that return futures. Since we are calling those methods directly in the various select arms, the futures returned by those methods are already dropped. Dropping these eagerly feels like unnecessary ceremony that doesn't actually reduce the risk of potential futurelock.
There was a problem hiding this comment.
To be clear, I think we should probably remove this, since it doesn't actually get us any additional safety. My concern about this sort of thing is that someone else might come along and read this code, and get the impression that doing this will protect us from futurelock, when it doesn't actually. You and I have spent enough time debugging this that we understand the problem fairly well, but someone who doesn't might think that this ritual will protect them when it won't, and then we've inadvertently created a superstition.
| // Futurelock safety: All select arms are queried concurrently. No | ||
| // awaiting happens outside this concurrent polling. |
There was a problem hiding this comment.
FWIW, this select is also futurelock-safe since it contains no arms where the future is an &mut future, so there is no un-dropped future that may have acquired a lock or similar resource. It might be worth mentioning that here, as well; I think part of the value of these "safety" comments is that they enumerate all of the things which, if someone were to change, they might introduce a problem.
| // Out of an abundance of caution, to avoid futurelock: drop all | ||
| // possible unpolled futures before invoking terminate. | ||
| drop(rebalance_interval); | ||
| drop(backend_status_stream); | ||
| drop(resolver_stream); |
There was a problem hiding this comment.
To be clear, I think we should probably remove this, since it doesn't actually get us any additional safety. My concern about this sort of thing is that someone else might come along and read this code, and get the impression that doing this will protect us from futurelock, when it doesn't actually. You and I have spent enough time debugging this that we understand the problem fairly well, but someone who doesn't might think that this ritual will protect them when it won't, and then we've inadvertently created a superstition.
| // Futurelock safety: All select arms are queried concurrently. No | ||
| // awaiting happens outside this concurrent polling. |
There was a problem hiding this comment.
This one I think is important because we are select!ing over an &mut future in a loop, which is a warning sign. I might also note that tick_and_query_dns does not interact with terminate_rx, although the fact that all that code runs in the select's future rather than the body of the select means it would be safe even if it did change to do that.
await-ing inselect!statementsDepends on #118