-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
MONGOID-5820 Fix the session object leak when using transactions #5878
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just one question about the commented-out bit in lite_spec_helper.rb, but I've got no material concerns.
spec/lite_spec_helper.rb
Outdated
# if SpecConfig.instance.ci? && !%w(1 true yes).include?(ENV['INTERACTIVE']&.downcase) | ||
# config.around(:each) do |example| | ||
# TimeoutInterrupt.timeout(example_timeout_seconds) do | ||
# example.run | ||
# end | ||
# end | ||
# end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these lines commented out because they might come back? Should we just delete them and rely on git to remember them if we need to resurrect them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, good catch! This should be removed eventually, I believe, but not in scope of this PR. I'll revert.
Hi @Virus-X , thank you very much for your contribution! I am sorry it takes me so much time to merge it. The reason was that we needed a test for the change; and while adding the test I discovered one more related issue that was also fixed. |
…godb#5878) Co-authored-by: Dmitry Rybakov <[email protected]>
…) (#5893) Co-authored-by: Valerii Goncharenko <[email protected]>
Thanks! Was glad to help! |
The issue
I found the memory leak inside
Threaded
class, when saving documents within transaction: session object is retained forever inside thread-local variable.Explanation
modified_documents
until transaction is committed.[mongoid]:modified-documents
contains a hash withsession
as key and Set of documents as value.Mongoid::Threaded::clear_modified_documents
is called, which clears documents from the set, however, the hash inside[mongoid]:modified-documents
still contains a session object as a key.Mongoid::Threaded::clear_session
call.Testing
I used the following code to reproduce the issue:
Initial results:
Report shows a number of retained objects, which corresponds to 2000 iterations of test:
![image](https://private-user-images.githubusercontent.com/2418413/375453545-036a00bf-8e04-4c2c-9af1-03d46ffaa662.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk2NDU5MTMsIm5iZiI6MTczOTY0NTYxMywicGF0aCI6Ii8yNDE4NDEzLzM3NTQ1MzU0NS0wMzZhMDBiZi04ZTA0LTRjMmMtOWFmMS0wM2Q0NmZmYWE2NjIucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI1MDIxNSUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNTAyMTVUMTg1MzMzWiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9MTRlMzk1NjI0N2E5YWYxMDJmNzQ0MTZiNTA1NDQzMTBhMzE1ZmViODljOTg0YTBmYThhNDViMzA3YTQzNTE2NiZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ.um0S77srb3ZTiSqm5V6s0AmnwE6KJwWiLEhkcFYdUnc)
Results after modification: