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

Track CoroutineContext in Invocation tag #3240

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

Conversation

AlessandroOddone
Copy link

@AlessandroOddone AlessandroOddone commented Nov 2, 2019

This enables accessing the CoroutineContext where the method was called from an OkHttp interceptor.

Use cases:

  • inspect elements of the CoroutineContext for logging/monitoring
  • use runBlocking from Interceptor.intercept while preserving elements from the calling CoroutineContext

@JakeWharton
Copy link
Member

@JakeWharton JakeWharton commented Nov 3, 2019

I'm not super eager about this since suspend is meant to behave like a call adapter–entirely opaque to the actual execution of the call. I'm not opposed to passing the context to the invocation as a nullable thing, though.

We'll also need a test to ensure this works correctly.

@AlessandroOddone AlessandroOddone changed the title Include Continuation in Invocation arguments Track CoroutineContext in Invocation tag Nov 4, 2019
@AlessandroOddone
Copy link
Author

@AlessandroOddone AlessandroOddone commented Nov 4, 2019

Updated. I added a kotlinCoroutineContext property to Invocation and a test.

@JakeWharton
Copy link
Member

@JakeWharton JakeWharton commented Nov 20, 2019

Right now the Kotlin dependency is optional, and this interacts poorly with that. Unfortunately we don't have a good test setup which covers this case. I still think is doable, but it probably mean we need to pass around things as Object internally.

@AlessandroOddone
Copy link
Author

@AlessandroOddone AlessandroOddone commented Nov 24, 2019

Right now the Kotlin dependency is optional, and this interacts poorly with that. Unfortunately we don't have a good test setup which covers this case. I still think is doable, but it probably mean we need to pass around things as Object internally.

I added a rule to exclude Kotlin dependencies from the classpath for test classes not named Kotlin*Test, and a test that ensures that CoroutineContext is actually not in the classpath when testing Invocation. This seems to work correctly in this scenario. I also validated this by integrating the snapshot in a Java-only project, without encountering any issues. Is my test case missing anything?

@danielrna
Copy link

@danielrna danielrna commented Jul 20, 2020

Has this issue been resolved somewhere else please ? I really need to use the coroutineContext in my Okhttp interceptor. Thanks (:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants