Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: respect shutdown in agent #31049

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

EdmondChuiHW
Copy link
Contributor

Summary

Listeners were not unsubscribed to upon shutdown. Discussed in #31024.

How did you test this change?

Test E2E in D63233256

Copy link

vercel bot commented Sep 24, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-compiler-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 24, 2024 5:17pm

@hoxyq
Copy link
Contributor

hoxyq commented Sep 24, 2024

The Bridge should already unsubscribe all listeners once shutdown event is emitted:

// Unsubscribe this bridge incoming message listeners to be sure, and so they don't have to do that.
this.removeAllListeners();
.

But I don't think that the Bridge on the other side intercepts this event. I see that Store does it, but Agent doesn't, exactly what you are mentioning.

Great catch, this can definitely be the root cause, but I guess we just need to somehow test this.

@hoxyq
Copy link
Contributor

hoxyq commented Sep 24, 2024

In any case, with the application frame reload, these Agent and Bridge are going to be destroyed, so there might be some small interval where we end up sending getProfilingStatus from the Frontend side, then the initial Bridge has enough time to respond with profilingStatus event, then the frame is reloaded, and the new Bridge sends profilingStatus again.

But I am not sure that this can actually happen, need to double-check the order between Store and Agent initialization.

@hoxyq
Copy link
Contributor

hoxyq commented Sep 24, 2024

I guess you can try reverting your changes in #31024 and see what happens with these changes.

hoxyq added a commit that referenced this pull request Oct 9, 2024
)

Based on #31049, credits to
@EdmondChuiHW.

What is happening here:
1. Once Agent is destroyed, unsubscribe own listeners and bridge
listeners.
2. [Browser extension only] Once Agent is destroyed, unsubscribe
listeners from BackendManager.
3. [Browser extension only] I've discovered that `backendManager.js`
content script can get injected multiple times by the browser. When
Frontend is initializing, it will create Store first, and then execute a
content script for bootstraping backend manager. If Frontend was
destroyed somewhere between these 2 steps, Backend won't be notified,
because it is not initialized yet, so it will not unsubscribe listeners
correctly. We might end up duplicating listeners, and the next time
Frontend is launched, it will report an issues "Cannot add / remove node
...", because same operations are emitted twice.

To reproduce 3 you can do the following:
1. Click reload-to-profile
2. Right after when both app and Chrome DevTools panel are reloaded,
close Chrome DevTools.
3. Open Chrome DevTools again, open Profiler panel and observe "Cannot
add / remove node ..." error in the UI.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants