-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Introduce ThreadContextElement API to integrate with thread-local sensitive code #454
Conversation
assertEquals(null, myThreadLocal.get()) | ||
job.join() | ||
assertEquals(null, myThreadLocal.get()) | ||
} |
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.
@Test
fun testUndispatched()= runTest {
val element = MyElement()
val job = launch(context = CommonPool + element, start = CoroutineStart.UNDISPATCHED) {
assertNotNull(myThreadLocal.get())
yield()
assertNotNull(myThreadLocal.get())
}
assertNull(myThreadLocal.get())
job.join()
assertNull(myThreadLocal.get())
}
This test fails
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.
Fixed it. Added test. Turned out there was a combination of bugs. Not only the test did not work, but actually none of the tests worked because test resources were not included into test builds, but those failures were silently ignored.
* // declare custom coroutine context element | ||
* class MyElement : AbstractCoroutineContextElement(Key) { | ||
* companion object Key : CoroutineContext.Key<MyElement> | ||
* // some state is kept here |
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.
[nitpick] worth adding some real value here, so it will be perfectly clear for users
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.
Updated examples.
I had to add lot more changes to clean up and fix the way thread context is installed. Test for debug thread names is also introduced. |
bbb0fe5
to
ea55c5b
Compare
@qwwdfsad I rewrote |
I've reworked it once more, getting rid of |
dd87ea0
to
8f7459e
Compare
@@ -172,10 +174,22 @@ private class LazyStandaloneCoroutine( | |||
} | |||
} | |||
|
|||
private class RunContinuationDirect<in T>( | |||
private class RunContinuationUndispatched<in T>( |
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.
maybe Unintercepted
for consistency?
* Now, `MyCoroutineContextThreadLocal` fully qualified class named shall be registered via | ||
* `META-INF/services/kotlinx.coroutines.experimental.CoroutineContextThreadLocal` file. | ||
*/ | ||
public interface CoroutineContextThreadLocal<E : CoroutineContext.Element, S> { |
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.
Please write a small section in our guide about threadlocals
override fun minusKey(key: CoroutineContext.Key<*>): CoroutineContext { | ||
return if (this.key == key) EmptyCoroutineContext else this | ||
} | ||
|
||
// this method is overridden to perform reference comparison on key |
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.
not to perform?
…sitive code * Debug thread name is redesigned using ThreadContextElement API where the name of thread reflects the name of currently coroutine. * Intrinsics for startCoroutineUndispatched that correspond to CoroutineStart.UNDISPATCHED properly update coroutine context. * New intrinsics named startCoroutineUnintercepted are introduced. They do not update thread context. * withContext logic is fixed properly update context is various situations. * DebugThreadNameTest is introduced. * Reporting of unhandled errors in TestBase is improved. Its CoroutineExceptionHandler records but does not rethrow exception. This makes sure that failed tests actually fail and do not hang in recursive attempt to handle unhandled coroutine exception. Fixes #119
* Move implementation to internal package * Add guide section
Fixes #119