[CP-beta][ Tool ] Add Stream.transformWithCallSite to provide more useful stack traces
#177994
Conversation
…ack traces (flutter#177470) Exceptions thrown within a stream transformer don't provide any context as to where the call to `transform(...)` occurred within the program, often resulting in stack traces consisting of only Dart SDK sources. This change adds a new extension method on `Stream` called `transformWithCallSite`, which captures the current `StackTrace` upon invocation. This stack trace is reported in the case of an error in order to provide context for better error reporting. Example issue: flutter#81666
|
This pull request was opened from and to a release candidate branch. This should only be done as part of the official Flutter release process. If you are attempting to make a regular contribution to the Flutter project, please close this PR and follow the instructions at Tree Hygiene for detailed instructions on contributing to Flutter. Reviewers: Use caution before merging pull requests to release branches. Ensure the proper procedure has been followed. |
|
@bkonyi please fill out the PR description above, afterwards the release team will review this request. |
github-actions
bot
added
platform-ios
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a Stream.transformWithCallSite extension method and a utf8LineDecoder utility to provide more useful stack traces when stream transformations fail. These changes are propagated throughout the flutter_tools codebase, improving debuggability. A new test is also added to verify the behavior of transformWithCallSite. The changes are generally good, but I've found one critical issue where the new utf8LineDecoder is used incorrectly, potentially causing issues with decoding adb logcat output.
| .transform<String>(decoder) | ||
| .transform<String>(const LineSplitter()) | ||
| .listen(_onLine); | ||
| _adbProcess.stdout.transform(utf8LineDecoder).listen(_onLine); |
There was a problem hiding this comment.
The stdout stream from adb logcat can contain invalid UTF-8, as noted in the comment on line 1119. Using utf8LineDecoder here is incorrect because it does not ignore decoding errors, which could lead to lost log output or crashes. You should apply transformWithCallSite manually with the existing decoder that has reportErrors: false, similar to how stderr is handled in this same block.
| _adbProcess.stdout.transform(utf8LineDecoder).listen(_onLine); | |
| _adbProcess.stdout.transformWithCallSite(decoder).transform(const LineSplitter()).listen(_onLine); |
There was a problem hiding this comment.
Code Review
This pull request introduces a Stream.transformWithCallSite extension method to provide more useful stack traces when stream transformations fail. It also adds a utf8LineDecoder stream transformer that combines UTF-8 decoding with line splitting, leveraging the new extension method. These changes have been applied across the codebase, replacing direct calls to utf8.decoder and LineSplitter with the new, more robust, and cleaner utilities. This is a solid improvement for debuggability and code maintainability. I have one suggestion for consistency.
There was a problem hiding this comment.
Code Review
This pull request introduces a Stream.transformWithCallSite extension method to provide more useful stack traces for stream errors, and a utf8LineDecoder utility for convenience. The changes refactor numerous parts of the codebase to use these new utilities, which should improve debuggability.
However, I've found a critical regression in packages/flutter_tools/lib/src/android/android_device.dart. The refactoring to use utf8LineDecoder for stdout from logcat incorrectly uses a UTF-8 decoder that does not handle malformed input. This could cause the tool to crash when processing logcat output, undoing a previous bug fix. The rest of the refactoring appears correct and beneficial.
This was incorrectly updated as part of flutter#177470
…14a28eb7fd919b638d191fe3
camsim99
added
the
autosubmit
auto-submit
bot
merged commit 5205556
into
flutter:flutter-3.38-candidate.0
…vide more useful stack traces (flutter/flutter#177994)
3 participants
This pull request is created by automatic cherry pick workflow
Please fill in the form below, and a flutter domain expert will evaluate this cherry pick request.
Issue Link:
What is the link to the issue this cherry-pick is addressing?
Example issue: #81666
Changelog Description:
Explain this cherry pick in one line that is accessible to most Flutter developers. See best practices for examples
Improve logging for UTF-8 decoding errors.
Impact Description:
What is the impact (ex. visual jank on Samsung phones, app crash, cannot ship an iOS app)? Does it impact development (ex. flutter doctor crashes when Android Studio is installed), or the shipping production app (the app crashes on launch)
When an exception is thrown within a transformer provided to
Stream.transform(...), an asynchronous stack trace pointing to the internal transformation machinery is reported instead of where the call totransform(...)was made, making tracking down where these exceptions come from extremely difficult.Workaround:
Is there a workaround for this issue?
No.
Risk:
What is the risk level of this cherry-pick?
Test Coverage:
Are you confident that your fix is well-tested by automated tests?
Validation Steps:
What are the steps to validate that this fix works?
Not a fix, just additional logging. Tests have been added to ensure this extension works as expected.