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

Add CoroutineHttpService for handle request within Coroutine suspend #5603

Merged
merged 6 commits into from
May 10, 2024

Conversation

pinest94
Copy link
Contributor

@pinest94 pinest94 commented Apr 12, 2024

Motivation:

Modifications:

  • Add CoroutineHttpService implements HttpService

Result:

@CLAassistant
Copy link

CLAassistant commented Apr 12, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@minwoox minwoox left a 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?
Copy link
Member

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:

public final class CoroutineContextService extends SimpleDecoratingHttpService {

You may want to refer to this code snippet to see how it's implemented:
val userContext = CoroutineContexts.get(ctx) ?: EmptyCoroutineContext

"/hello",
object : CoroutineHttpService {
override suspend fun suspendedServe(ctx: ServiceRequestContext, req: HttpRequest): HttpResponse {
return HttpResponse.of("hello world")
Copy link
Member

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:

@jrhee17 jrhee17 added this to the 1.29.0 milestone Apr 16, 2024
@pinest94
Copy link
Contributor Author

pinest94 commented Apr 24, 2024

Looks great! How about also adding extension methods in this PR as well? 😆

Does the extension method here refer to the extension function of the ServerBuilder?

@ikhoon
Copy link
Contributor

ikhoon commented May 2, 2024

Right. Should we add coroutineService for ServerBuilder, VirtualHostBuilder, and ContextPathServicesBuilder?

Server
  .builder()
  .coroutineService("/prefix", myCoroutineService)
  ...

@minwoox
Copy link
Member

minwoox commented May 9, 2024

Let's add the extension methods in the next PR. 😉
#5671

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @pinest94! Please address @minwoox comments.

Copy link
Member

@minwoox minwoox left a 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!

Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 👍 👍

@minwoox minwoox merged commit 43ab067 into line:main May 10, 2024
15 checks passed
@minwoox
Copy link
Member

minwoox commented May 10, 2024

Great job, @pinest94! 👍

Dogacel pushed a commit to Dogacel/armeria that referenced this pull request Jun 8, 2024
…ine#5603)

Motivation:
- Closes line#5442

Modifications:
- Add CoroutineHttpService implements HttpService

Result:
- Closes line#5442.
- You can now use `CoroutineHttpService` that runs your service in a coroutine scope.
Co-authored-by: minwoox <songmw725@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide an easy way to create an HttpService from a Kotlin coroutine
5 participants