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: save execution stack in query as string #15039

Merged
merged 1 commit into from
Nov 14, 2024

Conversation

durran
Copy link
Contributor

@durran durran commented Nov 13, 2024

Summary

While debugging some unexpected memory usage in a production cluster, we noticed that mongoose's Query object was constructing a new Error instance in order to be able to preserve the original stack when throwing an error after attempting to execute the query more than once. This is problematic in that until the stack is converted to a string, the error retains references to every object in the call site info. This PR changes the execution stack object to the actual stack string to avoid this problem. Since this variable appears intended to not be part of the public API it seems reasonable to change.

Examples
Screenshot 2024-11-13 at 22 25 56 copy

@durran
Copy link
Contributor Author

durran commented Nov 14, 2024

Note see related more detailed reasoning for a similar change in Node itself: nodejs/node#34103 (comment)

@vkarpov15
Copy link
Collaborator

We also noticed some performance issues with the _executionStack pattern here. I think this PR confirms my suspicions that we should just remove _executionStack and originalStack in Mongoose 9.0 because those cause performance overhead on every single query, and the original rationale is somewhat obsolete. The original rationale was to help users debug duplicate queries, but the most common cause of duplicate queries was mixing await and callbacks, like await Model.find({}, function(err, docs) {}). Given that Mongoose no longer supports callbacks, duplicate queries are significantly less likely.

TLDR; thanks for the PR, LGTM. But we will probably remove the _executionStack logic in our next major release.

@vkarpov15 vkarpov15 added this to the 8.8.2 milestone Nov 14, 2024
@vkarpov15 vkarpov15 merged commit 93dad98 into Automattic:master Nov 14, 2024
37 of 38 checks passed
vkarpov15 added a commit that referenced this pull request Nov 15, 2024
vkarpov15 added a commit that referenced this pull request Nov 15, 2024
fix: save execution stack in query as string
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants