Repository navigation
[android]: removes RegisterExternalTexture race condition - #192951
Conversation
github-actions
Bot
added
platform-android
gaaclarke
changed the title
![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 thread synchronization in AndroidContextDynamicImpeller to prevent callers on the platform thread from observing kImpellerAutoselect during startup, and adds a new suite of unit tests. The review feedback suggests avoiding side effects inside FML_DCHECK by using #ifndef NDEBUG preprocessor guards, refactoring a unit test to use fml::AutoResetWaitableEvent to prevent timing-based flakiness, and capturing a shared pointer by value in a lambda to guarantee its lifetime.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
andywolff
left a comment
andywolff
left a comment
There was a problem hiding this comment.
Looks good to me except for a few comments
| // window in which a re-entrant |WaitForSetup| would deadlock. Wrapped in a | ||
| // DCHECK so this bookkeeping compiles out of release builds along with the | ||
| // assert that consumes it. The store always evaluates to true. | ||
| FML_DCHECK((setup_thread_id_.store(std::this_thread::get_id(), |
There was a problem hiding this comment.
I agree with the bot's note about avoiding side effects inside FML_DCHECK, but its suggestion of NDEBUG isn't right. Would #if FML_DCHECK_IS_ON be more appropriate?
There was a problem hiding this comment.
It was a decision that has tradeoffs. Having the code in a matching macro is helpful. Having it in one statement avoids messy indentation and explicit closing semantics.
I added FML_DCHECK_IS_ON to block the field from ever accidentally being used in production code which I think gets to the root of the concern.
| // signal so a waiter can never observe a completed setup alongside a stale | ||
| // id and trip the assert in |WaitForSetup| spuriously. After this point the | ||
| // raster thread may freely call |RenderingApi|, which no longer blocks. | ||
| FML_DCHECK( |
There was a problem hiding this comment.
ditto about #if FML_DCHECK_IS_ON
| namespace { | ||
| constexpr auto kSetupDelay = std::chrono::milliseconds(50); | ||
| // Allow for timer granularity so the assertion does not flake. | ||
| constexpr auto kMinObservedWait = std::chrono::milliseconds(40); |
There was a problem hiding this comment.
Are kSetupDelay and kMinObservedWait unused now that RenderingApiBlocksUntilBackendChosenusesfml::AutoResetWaitableEvent` ? If so, please remove them
There was a problem hiding this comment.
Do we need to update this comment too to reflect the changed behavior?
Maybe something like
The construction of these objects is performed on the raster thread during [SetupImpellerContext]. Accessors such as [RenderingApi] block until this setup has completed.
There was a problem hiding this comment.
Done, I just deleted it. It didn't seem right for the state of the code before the change either. That info exists in the docstrings for methods.
|
Oops, last feedback response didn't compile on linux. Hey, @andywolff I just did the preprocessor guard like you suggest. I'm not a fan but not worth spending one more neuron on =) |
This reverts commit 5a336e2.
|
@andywolff psych, forgot we can't remove that field conditionally because of the way FML_DCHECK works, it still has to parse on release builds. I put the guard around the check too. |
andywolff
left a comment
andywolff
left a comment
There was a problem hiding this comment.
LGTM 3: The LGTM Rises
|
gaaclarke
added
the
autosubmit
auto-submit
Bot
removed
the
autosubmit
|
autosubmit label was removed for flutter/flutter/192951, because - The status or check suite Dashboard Checks has failed. Please fix the issues identified (or deflake) before re-applying this label. |
andywolff
left a comment
andywolff
left a comment
There was a problem hiding this comment.
LGTM 3: The LGTM Rises
LGTM 4: Return of the LGTM
andywolff
added
the
autosubmit
flutter-dashboard
Bot
removed
the
autosubmit
fixes #192776
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.