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.

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