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 xterm.js resource leaks in split pane #3409

Merged
merged 4 commits into from
Jan 18, 2019
Merged

Conversation

igorsdv
Copy link
Contributor

@igorsdv igorsdv commented Jan 10, 2019

Fixes #3408, fixes #3351, fixes #3281.

Repeated calls to Terminal.open() on the same instance register a number of event listeners each time, which leads to resource leaks and erroneous listener duplication. It's not clear to me if multiple calls to Terminal.open() is a use case supported by xterm.js -- if it is, this issue should be fixed there. In the meantime, this is an ugly workaround to release all the event listeners prior to calling Terminal.open() again, while preserving the state we need to keep.

First, we run the equivalent of Terminal.dispose() to release disposables and events on the Terminal object. After this, some event listeners on non-disposable properties of the Terminal object itself remain set, and we must handle those separately. This is important because the old event listeners indirectly close over the previous _parent elements (now detached), which hold references to unmounted React components and old props, which in turn refer to old Terminal objects representing closed panes and tabs. Thus a single unreleased listener can be enough to cause a significant resource leak.

This can be seen using the Developer Tools by going to Memory > Take Snapshot. Multiple non-GCed Term components and Terminal instances are reachable even when only one pane remains open.

Additionally, the necessary cleanup on TERM_GROUP_EXIT in lib/components/terms.js is moved from onRef() to componentWillReceiveProps(). This is because this.props.sessions is not yet updated when onRef() is called, and currently the delete statement is never executed.

@rauchg
Copy link
Member

rauchg commented Jan 10, 2019

@igorsdv this is absolutely amazing. Thank you so much!

// Release listeners and disposables
_core._disposables.forEach(d => d.dispose());
_core._disposables.length = 0;
_core._events = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

Not all listeners are installed when open is called, doing this on multiple objects will almost certainly break other components. Can the code be changed to call open once? I think it works fine if you open on some parent element and transfer that between split elements.

Also here is upstream request xtermjs/xterm.js#1323

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Tyriar Thanks! The only listener not registered in open that I noticed was InputHandler. In the end the object ends up with the same number of listeners as before the dispose calls. But yes I agree that calling open once and reattaching the container as you suggest would be better, not sure how much more complicated it is with React but it should be possible.

@igorsdv
Copy link
Contributor Author

igorsdv commented Jan 12, 2019

Implemented @Tyriar's suggestion to reattach the xterm.js parent element without calling open again, in the end it turned out less intrusive than I anticipated. The performance gains are still there, but without messing with xterm.js internals.

@chabou
Copy link
Collaborator

chabou commented Jan 18, 2019

Awesome 🎉

Thanks for your assist @Tyriar

@chabou chabou merged commit 3881703 into vercel:canary Jan 18, 2019
@chabou chabou mentioned this pull request Jan 20, 2019
@chabou
Copy link
Collaborator

chabou commented Jan 23, 2019

Removing this.termRef is a breaking change
https://github.com/zeit/hyper/pull/3409/files#diff-268710614c2274f611be667a5f72bb13L79

It has broken some plugins: chabou/hyper-pane#42

@igorsdv igorsdv mentioned this pull request Jan 24, 2019
@bet4it bet4it mentioned this pull request Jun 27, 2019
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants