-
Notifications
You must be signed in to change notification settings - Fork 29.2k
Set max request pool size for DDC module loader when in CI #170565
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Possible fix for flutter#169574 It looks like the timeouts seem to be specific to downloading scripts. Specifically, after a certain point, we no longer download any scripts. Lowering the number of max concurrent requests seems to avoid Chrome timing out.
I'm going to try seeing if I can repro any timeouts with this max queue size in my test/logging PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and thanks for cleaning up all the globals
usage!
packages/flutter_tools/test/general.shard/web/devfs_web_ddc_modules_test.dart
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Hopefully this gets the tests running reliably.
Since #170565 the `Linux web_tool_tests` task has been green: - https://ci.chromium.org/ui/p/flutter/builders/luci.flutter.staging/Linux%20web_tool_tests?limit=100 - https://flutter-dashboard.appspot.com/#/build?taskFilter=Linux+web_tool&showBringup=true Fixes #169574
…70565) Possible fix for flutter#169574 It looks like the timeouts seem to be specific to downloading scripts. Specifically, after a certain point, we no longer download any scripts. Lowering the number of max concurrent requests seems to avoid Chrome timing out. This does a refactor as well to make sure we're piping FileSystem, Logger, and Platform into devfs_web.dart instead of relying on globals every time. This also fixes a test issue where we were always passing `isWindows: false` in some test cases instead of checking for the value. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing.
Since flutter#170565 the `Linux web_tool_tests` task has been green: - https://ci.chromium.org/ui/p/flutter/builders/luci.flutter.staging/Linux%20web_tool_tests?limit=100 - https://flutter-dashboard.appspot.com/#/build?taskFilter=Linux+web_tool&showBringup=true Fixes flutter#169574
Possible fix for #169574
It looks like the timeouts seem to be specific to downloading scripts. Specifically, after a certain point, we no longer download any scripts. Lowering the number of max concurrent requests seems to avoid Chrome timing out.
This does a refactor as well to make sure we're piping FileSystem, Logger, and Platform into devfs_web.dart instead of relying on globals every time. This also fixes a test issue where we were always passing
isWindows: false
in some test cases instead of checking for the value.Pre-launch Checklist
///
).