Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@Hixie
Copy link
Contributor

@Hixie Hixie commented Sep 9, 2020

No description provided.

@Hixie Hixie force-pushed the drain branch 2 times, most recently from 083ae23 to b3e48ce Compare September 10, 2020 18:34
Hixie added a commit to Hixie/flutter that referenced this pull request Sep 10, 2020
…ation.

Having side-effects in a getter is surprising and unintuitive. This reduces the side-effects caused by RestorationManager.rootBucket.

Also, flutter/engine#21062 is planning on making the message queue draining asynchronous, so we need to make sure that we subscribe to messages before the build phase is scheduled, otherwise we will be delaying the first frame by one cycle.
@Hixie
Copy link
Contributor Author

Hixie commented Sep 10, 2020

Test failures will be addressed by flutter/flutter#65579

@Hixie Hixie force-pushed the drain branch 3 times, most recently from d3c06e2 to 2fbea64 Compare September 17, 2020 19:50
@Hixie
Copy link
Contributor Author

Hixie commented Sep 17, 2020

This is blocked because it exposes a number of issues with ChannelBuffers that result in an infinite loop in one of the framework tests.

  1. drain is async, but if a message comes in while draining, it might be delivered before drain is finished (unordered) -- possible solution is to push messages onto the queue if they come in when the queue is non-empty.
  2. setting a handler to null still drains the queue -- this should just not happen.
  3. drain will handle messages that are added while it's running, so you can end up in an infinite loop, which is especially noticeable if the handler is removed while you're draining since then the framework will just keep putting each message back onto the queue.
  4. drain being called twice at the same time will result in two simultaneous drains to different callbacks.

We could avoid all of these by removing the concept of a ring buffer (flutter/flutter#66071), but that's a potentially very breaking change.

@Hixie
Copy link
Contributor Author

Hixie commented Sep 17, 2020

flutter/flutter#66073 probably unblocks this enough for now.

@Hixie Hixie marked this pull request as ready for review September 23, 2020 04:22
@Hixie
Copy link
Contributor Author

Hixie commented Sep 23, 2020

cc @gaaclarke @yjbanov This is ready for review now, with the framework-side changes having landed.

@Hixie Hixie added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Sep 24, 2020
@fluttergithubbot fluttergithubbot merged commit cdc631c into flutter:master Sep 24, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 24, 2020
Hixie added a commit that referenced this pull request Sep 24, 2020
renyou pushed a commit that referenced this pull request Sep 25, 2020
@Hixie
Copy link
Contributor Author

Hixie commented Sep 26, 2020

This failed in google3, will follow-up in flutter/flutter#66699.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants