-
Notifications
You must be signed in to change notification settings - Fork 926
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
Add CoroutineHttpService for handle request within Coroutine suspend #5603
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.
Looks great!
How about also adding extension methods in this PR as well? 😆
* invoke the suspendedServe method within the CoroutineScope | ||
*/ | ||
override fun serve(ctx: ServiceRequestContext, req: HttpRequest): HttpResponse { | ||
return HttpResponse.of(CoroutineScope(ctx.asCoroutineContext()).future { // Do we also need to add user context? |
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.
Let's also support the userContext that maybe set by a CoroutineContextService
:
armeria/kotlin/src/main/java/com/linecorp/armeria/server/kotlin/CoroutineContextService.java
Line 54 in d9a50e8
public final class CoroutineContextService extends SimpleDecoratingHttpService { |
You may want to refer to this code snippet to see how it's implemented:
armeria/kotlin/src/main/kotlin/com/linecorp/armeria/internal/common/kotlin/ArmeriaCoroutineUtil.kt
Line 152 in 655d48c
val userContext = CoroutineContexts.get(ctx) ?: EmptyCoroutineContext |
kotlin/src/main/kotlin/com/linecorp/armeria/server/kotlin/CoroutineHttpService.kt
Outdated
Show resolved
Hide resolved
"/hello", | ||
object : CoroutineHttpService { | ||
override suspend fun suspendedServe(ctx: ServiceRequestContext, req: HttpRequest): HttpResponse { | ||
return HttpResponse.of("hello world") |
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.
Let's check if the context is propagated correctly.
You might want to refer to this implementation:
Line 380 in df826c0
assertContextPropagation() |
Does the extension method here refer to the extension function of the |
Right. Should we add Server
.builder()
.coroutineService("/prefix", myCoroutineService)
... |
Let's add the extension methods in the next PR. 😉 |
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.
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.
Looks good, thanks @pinest94!
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.
👍 👍 👍
Great job, @pinest94! 👍 |
Motivation:
HttpService
from a Kotlin coroutine #5442Modifications:
Result:
HttpService
from a Kotlin coroutine #5442.CoroutineHttpService
that runs your service in a coroutine scope.