Skip to content

Conversation

srujzs
Copy link
Contributor

@srujzs srujzs commented Jun 13, 2025

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

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I signed the [CLA].
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is [test-exempt].
  • I followed the [breaking change policy] and added [Data Driven Fixes] where supported.
  • All existing and new tests are passing.

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.
@srujzs srujzs requested review from mdebbar and nshahan June 13, 2025 00:53
@github-actions github-actions bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Jun 13, 2025
@srujzs
Copy link
Contributor Author

srujzs commented Jun 13, 2025

I'm going to try seeing if I can repro any timeouts with this max queue size in my test/logging PR.

Copy link
Contributor

@mdebbar mdebbar left a 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!

Copy link
Contributor

@nshahan nshahan left a 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.

@mdebbar mdebbar added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 13, 2025
@auto-submit auto-submit bot added this pull request to the merge queue Jun 16, 2025
Merged via the queue into flutter:master with commit 3f2e8ba Jun 16, 2025
136 of 137 checks passed
@flutter-dashboard flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jun 16, 2025
mboetger pushed a commit to mboetger/flutter that referenced this pull request Jul 21, 2025
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants