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

Don't leak the viewport subscription when the viewport data is read #4420

Merged
merged 4 commits into from
Oct 25, 2023

Conversation

niloc132
Copy link
Member

@niloc132 niloc132 commented Aug 31, 2023

Fixes #4410

mattrunyon
mattrunyon previously approved these changes Aug 31, 2023
Copy link
Contributor

@mattrunyon mattrunyon left a comment

Choose a reason for hiding this comment

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

Looks like it's working properly in the docs snapshots

@niloc132
Copy link
Member Author

niloc132 commented Sep 1, 2023

This is on hold to address a few other leaks as well.

@niloc132 niloc132 closed this Sep 1, 2023
@niloc132 niloc132 modified the milestones: August 2023, September 2023 Sep 1, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Sep 1, 2023
@niloc132 niloc132 reopened this Sep 1, 2023
@niloc132 niloc132 modified the milestones: September 2023, October 2023 Oct 20, 2023
Copy link
Contributor

@mattrunyon mattrunyon left a comment

Choose a reason for hiding this comment

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

Should this close #4410?

Tested w/ docs snapshot tool by undoing the mitigations I had put in.

Confirmed w/ latest docker image the number of listeners grows unbounded by creating my own class extending ws which increments/decrements on newListener and removeListener.

With this change, number of listeners was not growing and stayed at a consistent overhead of 12 even after closing/re-opening the connection.

@niloc132
Copy link
Member Author

Fixed the linked issue, thanks.

@niloc132 niloc132 merged commit de313bb into deephaven:main Oct 25, 2023
10 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JS API getViewportData does not cleanup websocket listeners on table close
2 participants