-
Notifications
You must be signed in to change notification settings - Fork 395
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
Capture throwable inside generateDocumentation thread #2935
Capture throwable inside generateDocumentation thread #2935
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.
Wow, thanks for noticing and fixing it!
var throwableInGenerate: Throwable? = null | ||
/** | ||
* Run in a new thread to avoid memory leaks that are related to ThreadLocal (that keeps `URLCLassLoader`) | ||
* Currently, all `ThreadLocal`s leaking are in the compiler/IDE codebase. | ||
*/ | ||
Thread { generate() }.apply { | ||
Thread { | ||
try { | ||
generate() | ||
} catch (t: Throwable) { | ||
throwableInGenerate = t | ||
} | ||
}.apply { | ||
start() | ||
join() | ||
} | ||
throwableInGenerate?.let { throw it } |
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.
I'm not sure why Kotlin allows writing such code and what exactly happens "under the hood" when this is run on a JVM. Is it thread safe? I can't say for sure...
I do know that you cannot write such code in Java, it requires the used variable to be either final or effectively final for good reasons.
To be on the safe side, I think we can use AtomicReference
to hold the throwable, then it should be thread safe.
While at it, we could use UncaughtExceptionHandler
(which actually catches Throwable
) instead of the try-catch, but it's optional :)3
To put the words into code:
val uncaughtExceptionHolder = AtomicReference<Throwable?>()
/**
* Run in a new thread to avoid memory leaks that are related to ThreadLocal (that keeps `URLCLassLoader`)
* Currently, all `ThreadLocal`s leaking are in the compiler/IDE codebase.
*/
Thread { generate() }.apply {
setUncaughtExceptionHandler { _, throwable ->
uncaughtExceptionHolder.set(throwable)
}
start()
join()
}
uncaughtExceptionHolder.get()?.let { throw it }
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.
Thanks a lot for looking into it and providing such a detailed guidance.
I'm not sure why Kotlin allows writing such code and what exactly happens "under the hood" when this is run on a JVM. Is it thread safe? I can't say for sure...
I took a look at the bytecode generated (see https://godbolt.org/z/6d47nPs4a) and under the hood is using ObjectRef
, which is a static final class
that holds an element
.
That allows the use of var throwableInGenerate
even though it's not really final.
Anyway, that doesn't guarantee thread safety. So your proposed solution using AtomicReference
does indeed help with ensuring that the throwable
is visible to all threads and it is set atomically.
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, it's nice to know what it's being compiled into, thanks for looking into it and sharing the findings :)
… internal thread
905abc0
to
a1f2a2c
Compare
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.
Perfect, thank you very much for the fix, and sorry it took so long to get merged!
Capture any throwable thrown by the
generate()
function running in a separate thread to later on throw it in the context of thegenerateDocumentation()
calling thread.Fixes #2934