Do not permanently attach JVM threads#185
Conversation
|
(Following up, as you asked us to try this out in the other issue.) Currently, I'm not seeing any JVM heap size growth in my testbed app that exhibited the issue earlier after thirty minutes of running. (The test bed app is a simple app that opens a connection that uses this library every few seconds or so, and then closes it down, and tries again in a loop. Before this, we would see unbounded heap growth until an eventual OOM crash as the verifier ran on a worker thread from a thread pool, and the other tasks on the thread could depend on the JVM de-attach to clean up their memory usage, as Jordan discussed in #184.) I plan to run the app over night in a "soak test", but this seems to be an early indication that this does in fact fix the issue, as expected. Thanks again for the quick fix! |
djc
left a comment
djc
left a comment
There was a problem hiding this comment.
Weak approval: I think this makes sense but I don't have much expertise in JVM matters to review this thoroughly.
complexspaces
added
bug
banasiak
left a comment
banasiak
left a comment
There was a problem hiding this comment.
Hello friends. @complexspaces asked me to take a look at this PR. I agree that this is a good idea for the library as it will allow the thread to be released when it goes out of scope. 👍
|
I took a look at the CI test failures:
|
88c93db to
97fd714
Compare
We are a library and may be used in any kind of threading context, so leave this performance choice up to the caller. Additionally, optimize env access patterns to be more clear how many times we try attaching.
97fd714 to
6c74f85
Compare
|
I've rebased this on |
4 participants
We are a library and could be used in any kind of threading context, so we should leave the thread attachment behavior (a performance optimization) choice up to the caller. I did, however, also reduce the number of attachments we call down to 1 as a performance optimization since the existing patterns we have allow it to be safe.
Closes #184