Skip to content

Conversation

@christopherfujino
Copy link
Contributor

@christopherfujino christopherfujino commented Mar 11, 2022

Fixes #99021
Related to #99406
Related to #98421
Related to #99855

@flutter-dashboard flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Mar 11, 2022
@christopherfujino christopherfujino changed the title Close stream [flutter_tools] check if stream is open before sending message Mar 16, 2022
@christopherfujino christopherfujino changed the title [flutter_tools] check if stream is open before sending message [flutter_tools] check if stream is open before sending message in ios device Mar 16, 2022
@christopherfujino christopherfujino marked this pull request as ready for review March 16, 2022 21:58
onError: _linesController.addError,
onDone: _linesController.close,
(String line) {
if (!linesController.isClosed) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the actual fix, the other added if checks are just defensive.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why here and not in e.g. _newSyslogLineHandler(), or up on line 697?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were there multiple stacktraces we're seeing in CI, or is this a more general question? The stacktraces I saw where all on line 719.

Should I wrap the StreamController so that every .add() call has a check first if the delegate is closed?

Copy link
Contributor Author

@christopherfujino christopherfujino Mar 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or I can add a new method on the IOSDeviceLogReader class

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

@Jasguerrero Jasguerrero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fluttergithubbot fluttergithubbot merged commit e99a66a into flutter:master Mar 17, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 17, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Mar 18, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Mar 18, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Mar 18, 2022
@christopherfujino christopherfujino deleted the close-stream branch March 22, 2022 02:01
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.

Bad state: Cannot add new events after calling close, IOSDeviceLogReader.debuggerStream= > _BroadcastStreamController.add

4 participants