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

Reset runtime context options before releasing it back into pool #500

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

lovromazgon
Copy link

@lovromazgon lovromazgon commented Feb 28, 2024

This PR makes sure RuntimeContext options are reset before the context is returned to the pool.

It also adds a test case that fails without the change.

Fixes #499.

@lovromazgon
Copy link
Author

@goccy any chance you could look at the issue and PR? To me, it seems like a pretty straightforward fix.

@goccy
Copy link
Owner

goccy commented May 9, 2024

Sorry for the late reply, Please re-commit to run CI 🙏

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.53%. Comparing base (df897ae) to head (abd07bc).
Report is 4 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #500      +/-   ##
==========================================
+ Coverage   76.78%   78.53%   +1.74%     
==========================================
  Files          55       55              
  Lines       18922    16714    -2208     
==========================================
- Hits        14529    13126    -1403     
+ Misses       3759     2956     -803     
+ Partials      634      632       -2     

@lovromazgon
Copy link
Author

lovromazgon commented May 21, 2024

@goccy I see you released 0.10.3, is there anything missing here that prevented you from including this PR?

@goccy
Copy link
Owner

goccy commented May 21, 2024

I saw this PR, but I cannot accept this implementation.

@lovromazgon
Copy link
Author

lovromazgon commented May 21, 2024

Can you elaborate? Do you have suggestions on other approaches to address this bug?

EDIT: I assume you're concerned about the allocation. How about the approach with setting the context to nil before releasing it?

@lovromazgon
Copy link
Author

Any chance you had a look at the last commit @goccy? Or at least describe your concerns so I know how to address them.

@lovromazgon
Copy link
Author

@goccy can you please respond? I am willing to do the work and fix the bug, but I can't do it without your cooperation.

@lovromazgon
Copy link
Author

A friendly reminder that I'm waiting for a response.

@lovromazgon
Copy link
Author

Bump.

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.

Runtime context is not reset before released to the pool
3 participants