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: use single cancellation listener in Open AI model #14914

Closed

Conversation

sdirix
Copy link
Member

@sdirix sdirix commented Feb 13, 2025

What it does

Adjusts the OpenAiModel to use a single cancellation listener instead of adding a new one for every received chunk. This fixes the warning about a possible emitter memory leak when receiving long responses from the Open AI models.

fixes #14902

How to test

Work with any Open AI model. For example use an empty system prompt for @Universal and ask it to tell a long story.

You should observe that the warnings about a possible emitter leak no longer appear.

root WARN Possible Emitter memory leak detected. 1320 listeners added. Use event.maxListeners to increase the limit (175). MOST frequent listener (1145):
root WARN     at _event.Object.assign.maxListeners (...theia/examples/electron/lib/backend/packages_core_lib_common_index_js-node_modules_vscode-languageserver-types_lib_umd_sync_recursive.js:1456:54)
    at ...theia/examples/electron/lib/backend/main.js:6599:113

Follow-ups

I did check the other model implementations but did not see the same bad pattern used there.
In many places we add a single listener which is not disposed later, but that should be fine as we also don't expect the cancellation token to live forever. It's just that this case was way too extreme, adding a listener per chunk.

Breaking changes

  • This PR introduces breaking changes and requires careful review. If yes, the breaking changes section in the changelog has been updated.

Attribution

Review checklist

Reminder for reviewers

Adjusts the 'OpenAiModel' to use a single cancellation listener instead
of adding a new one for every received chunk. This fixes the warning
about a possible emitter memory leak when receiving long responses from
the Open AI models.

fixes eclipse-theia#14902
Copy link
Contributor

@eneufeld eneufeld left a comment

Choose a reason for hiding this comment

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

Looks good. I don't have any warning anymore.

Comment on lines +144 to +145
resolve = undefined;
reject = undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

See my comments re: the current pinned resolver approach in #14920. Let's decide whether it could really cause the problems I think it could before we move forward.

@sdirix
Copy link
Member Author

sdirix commented Feb 14, 2025

Hi @colin-grant-work, I had a look at your PR and I think overall yours is the better solution. There is nothing wrong (at the moment) with the code here, as we have a synchronous consumer, still your code is better encapsulated and actually enables testing in a nice way. So let's proceed with #14920

@sdirix sdirix closed this Feb 14, 2025
@sdirix sdirix deleted the fix-max-listener-warning branch February 14, 2025 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Listener memory leak in AI system
3 participants