Reject requests for hot reload if a hot reload is already in progress. #14494
Conversation
|
Oh dear, looks like I didn't fix my lints! :( Will sort these first in the morning. |
devoncarew
left a comment
devoncarew
left a comment
There was a problem hiding this comment.
Very nice!
Also; should I be adding run_machine_concurrent_hot_reload to the manifest file?
Yes, I think so; that way this test will run as part of the devicelab tests.
| vmServicePort = Uri.parse(json['params']['wsUri']).port; | ||
| print('service protocol connection available at port $vmServicePort'); | ||
| } | ||
| else if (json != null && json['event'] == 'app.started') { |
There was a problem hiding this comment.
nit: else on the previous line
Since this is a new file, you may run it through dartfmt once in order to level set the formatting (you probably don't want to do that to existing repo files however)
There was a problem hiding this comment.
Fixed in 55b68ef (note: I'm not sure if linking to changesets in response to comments is useful or spammy, it's just something I did at previous company, lmk if not useful!)
| <String>['run', '--machine', '--verbose', '-d', device.deviceId, 'lib/commands.dart'], | ||
| ); | ||
| final StreamController<String> stdout = new StreamController<String>.broadcast(); | ||
| run.stdout |
There was a problem hiding this comment.
We're doing this twice in this file (and lots of places in other files); perhaps in-line a small Stream<List<int>> ==> Stream<String function?
Stream<String> transformToLines(Stream<List<int>> byteStream) {
return byteStream.transform(UTF8.decoder).transform(const LineSplitter());
}
There was a problem hiding this comment.
In 859a279 I made a function at the top, though not sure if you want more globally?
This is one of those places I wish Dart had extension methods... stream.transformToLines() versus transformToLines(stream)!
| bool isRestartSupported(bool enableHotReload, Device device) => | ||
| enableHotReload && device.supportsHotMode; | ||
|
|
||
| Future<Null> inProgressHotReload; |
There was a problem hiding this comment.
nit: newline after
Perhaps make this private? _inProgressReload?
There was a problem hiding this comment.
Done in ae25a98. C# is private by default, suspect I will keep making this mistake for a while!
| throw "app '$appId' not found"; | ||
|
|
||
| return app._runInZone(this, () { | ||
| if (!fullRestart && inProgressHotReload != null) |
There was a problem hiding this comment.
We probably want to disallow any concurrent reload/restart? So, if (_inProgressReload != null) ...
There was a problem hiding this comment.
Changed in b01335d. I wasn't totally sure if we wanted to affect full restarts too, but I guess it could suffer from similar issues so seems safest (as long as we don't ever go leaving this future around when it should've been cleared - I think hot reload has a timeout so hopefully this is all handled nicely).
| }); | ||
| if (!fullRestart) | ||
| inProgressHotReload = action.then((_) => inProgressHotReload = null); | ||
| return action; |
There was a problem hiding this comment.
It may be possible to simplify this all to:
_inProgressReload = app._runInZone(this, () {
return app.restart(fullRestart: fullRestart, pauseAfterRestart: pauseAfterRestart);
});
return _inProgressReload.whenComplete(() {
_inProgressReload = nu'l;
});
In which case we'd want to type _inProgressReload as Future<OperationResult>. I think the above will have the nice benefit that errors in the reload operation future will be passed back to the caller (and back over the wire).
There was a problem hiding this comment.
Oops, good catch - I think my original code would've failed to set the in-progress future back to null if the runInZone future errorred. 2a58cd8 changes to your code (similar to the earlier change, this removes the checking for only hot reloads, so the future will be set for full restarts too, but I think you intended this).
I've added this in b406480, I'm not sure if these values are set optimally: stage: devicelab
required_agent_capabilities: ["mac/android"]I've been building/testing on windows/android though the test I originally copied was set to mac/android. In theory the test should work fine on all platforms, but I don't know if there's a trade-off between running it in all places versus how long the test run will take. |
... but undo things that cause lints (like single-line ifs)
devoncarew
left a comment
devoncarew
left a comment
There was a problem hiding this comment.
Looks good! Let me know if you can merge; if not, I can do for now.
I'd watch the devicelab build page after this lands to make sure everything looks good.
|
@devoncarew I can't merge; I don't have any access here (which might not be a bad thing, at least for now! 😁). I'll keep an eye on the devicelab tests once merged 👍 |
Looks like everything (eventually) went green, thanks! |
|
Just realized that I forgot to click the "Send" button 🤦♂️ I was going to say that when we add a brand new devicelab test we begin by marking it as |
Hmm, unsure if misplaced faith or sarcasm! 😛 But I can see on the build page that two runs are red and two are yellow. I'm not entirely sure what's with the yellow ones - is is being run multiple times to determine it's flaky? I can't currently access the logs (I don't have access to my @google account yet, and my personal Google account gives |
|
@mraleph I think everyone above may be on US time so I wonder if you have any opinions on this - my devicelab test |
|
I got some of these logs from @mraleph; I guess something is trying to write to stdout as it's terminating (as a result of So, two questions:
NoSuchMethodError: The getter 'stdout' was called on null. |
|
Looks like it's trying to print But it looks like you're tickling an existing issue - this isn't new with your PR. |
|
@tvolkert, @jcollins-g, is it expected that |
|
|
|
I've raised a specific issue to investigate/fix this flakiness (#14636) since this PR is already merged and it could easily get lost. |
| '-d', | ||
| device.deviceId, | ||
| 'lib/commands.dart' | ||
| ], |
There was a problem hiding this comment.
Is this supposed to support preview-dart-2 mode?
Not sure if this changes are for preview-dart-2, but if they are the tests should run together with other preview-dart-2 hot reload tests. |
|
No, this one is just for a crash when concurrent reload requests are sent (it existed well before preview-dart-2). I've since been told about reload issues with preview-dart-2 via the daemon though, so suspect there will be additional daemon tests on the way. |
flutter#14494) * Reject requests for hot reload if a hot reload is already in progress. Fixes flutter#14184 * Implement TODO, verifying further hot reloads complete sucessfully. * Fix year on new file. * Add missing type annotations to fix lints * Add run_machine_concurrent_hot_reload to manifest for CI * Reformat document ... but undo things that cause lints (like single-line ifs) * Extract std stream transformations * Make inProgressHotReload private * Disallow all types of reload while hot reload in progress * Simplify code handling in-progress hot reloads
6 participants
Fixes #14184
I created a copy of the
command_tests.dartdevicelab test and changed it to talk--machine(including extracting the debug port fromapp.debugPortsince there's no banner likeflutter run --hotwhich is what the other test used). Without the change indaemon.dartthis test will fail because of a crash. With the code change the test passes (the second reload is rejected).I'm somewhat nervous about this change because breaking hot reload would be a big deal, so please review carefully. Is the change in
daemon.dartreliable enough that it can't end up with a hungover future that stops future hot reloads working?Also; should I be adding run_machine_concurrent_hot_reload to the manifest file? The doc says "The
manifest.yamlfile describes a subset of tests we run in the CI" but I'm not certain what that subset is and whether this test should be part of it.As always, nitpick away; this is a learning exercise!
cc @devoncarew @yjbanov