Track futures to give better errors #462
Conversation
| disableBreakpoints: disableBreakpoints, | ||
| )); | ||
| return _trackFuture( | ||
| 'invoke $selector', |
There was a problem hiding this comment.
I like tracking the selector here!
| if (allFuturesCompleted.isCompleted) { | ||
| allFuturesCompleted = Completer<bool>(); | ||
| Future<T> _trackFuture<T>(String name, Future<T> future) { | ||
| final trackedFuture = new TrackedFuture(name, future); |
There was a problem hiding this comment.
nit: keep a map from Future to debug description instead of creating the TrackedFuture class.
that will make it a little easier to toggle this on and off.
There was a problem hiding this comment.
I made this change, however it occurs to me that if we want to disable tracking wouldn't we be better just skipping the whole trackFuture call? As far as I can tell, it's only tests that ever use the tracked futures, so if we want to disable them for production I think it would be better to not track them at all, rather than track them and just don't keep their descriptions? (in which case, I think going back to TrackedFuture would be better than keeping two lists in-sync). WDYT?
There was a problem hiding this comment.
I've reverted the Map and left it as it was, but with a bool to opt-in to tracking only for tests. I think this is better (keeps the code clean instead of having a list and a map to track, while still avoiding any tracking in production). PTAL!
| Completer<bool> allFuturesCompleted = Completer<bool>(); | ||
| final Set<TrackedFuture<Object>> activeFutures = Set(); | ||
| Completer<bool> _allFuturesCompleter = Completer<bool>(); | ||
| Future<void> get allFuturesCompleted => _allFuturesCompleter.future; |
|
|
||
| VmService _vmService; | ||
| final Map<String, Future<Success>> _activeStreams = {}; | ||
|
|
There was a problem hiding this comment.
add a top level bool field to this file like
String debugFutures;
that toggles on and off whether we track the Map<Future, String> futureCreators;
There was a problem hiding this comment.
It would probably make sense to set
debugFutures=true
for all our tests.
Outside of tests, this functionality isn't really useful like you say.
| if (_beforeTearDown != null) await _beforeTearDown(); | ||
|
|
||
| await _service.allFuturesCompleted.future; | ||
| await _service.allFuturesCompleted.timeout(Duration(seconds: 20), |
There was a problem hiding this comment.
This is really fantastic. Good idea making this less of a blackbox.
DanTup
force-pushed
the
log-tracked-futures
branch
from
085d19f to
5b617cb
Compare
|
@jacob314 Are you happy with the changes? |
DanTup
force-pushed
the
log-tracked-futures
branch
from
8e32f48 to
ad428bf
Compare
DanTup
force-pushed
the
log-tracked-futures
branch
from
ad428bf to
f2c4859
Compare
|
@devoncarew @jacob314 I'm getting this error on Travis:
However when I changed this to a collection literal, I then got:
Two questions:
|
|
I think we need to weak our environment constraint in pubspec.yaml. Supporting >= 2.2.0-dev is probably fine at this point and if we needed to supporting >= 2.3.0-dev would probably work. |
|
@pq see Danny's issue with getting prefer_collection_literals when it is not possible to use them as using them will trigger sdk_version_set_literal. It would be nice if the lints still played nicely with a wider sdk constraint for cases where it doesn't add significant complexity. For example, naively I would think that lints for new features should only trigger if the sdk constraints ensure the new feature can be used. |
This reverts commit f28feff.
DanTup
force-pushed
the
log-tracked-futures
branch
from
f05e3b5 to
7fcba27
Compare
This reverts commit 7625cb8.
|
Oh yeah. Thanks for looping me in @jacob314 . This is a known issue. As a general rule lints are promised to work with the version of the SDK they ship in and so there's a kind of implicit expectation that the SDK lower bound tracks that. In the case of dev releases this gets especially tricky though for a bunch of reasons. Happy to chat more offline... Sorry for the confusion in the meantime! cc @bwilkerson fyi |
3 participants
This is the code I was using to track which futures aren't being completed. Do you think we should land it? (it's slightly weird that this is in non-test code, but since we're already tracking them, I don't think adding a name to them is much of an issue).
(I also added a getter to avoid having access to the completer outside this class as that felt a bit weird).
I'll open another issue about the specific things this is logging, this PR is just to see if you think it's worth having this in the repo always, to help diagnose failures in the future (previously it'd just hang on the teardown on the
await).