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

Provide an easy way to create an HttpService from a Kotlin coroutine #5442

Closed
Dogacel opened this issue Feb 6, 2024 · 3 comments · Fixed by #5603
Closed

Provide an easy way to create an HttpService from a Kotlin coroutine #5442

Dogacel opened this issue Feb 6, 2024 · 3 comments · Fixed by #5603
Labels
new feature sprint Issues for OSS Sprint participants

Comments

@Dogacel
Copy link
Contributor

Dogacel commented Feb 6, 2024

Similar to

We should have HttpService based on coroutines. Currently, only way to implement a suspend http service is using AnnotatedService. In some scenarios, users might want to use the HttpService or AbstractHttpService, which in my opinion has more granularly defined signatures which seem less like magic.

Sample proposed implementation 1 (inheritance)

abstract class CoroutineHttpService : HttpService {
    final override fun serve(ctx: ServiceRequestContext, req: HttpRequest): HttpResponse {
        val future = CoroutineScope(ctx.asCoroutineContext()).future {
            serveAsync(ctx, req)
        }

        return HttpResponse.of(future)
    }

    abstract suspend fun serveAsync(ctx: ServiceRequestContext, req: HttpRequest): HttpResponse
}

Sample proposed implementation 2 (composition)

interface CoroutineHttpService {
    fun serve(ctx: ServiceRequestContext, req: HttpRequest): HttpResponse
}

fun CoroutineHttpService.asHttpService(): HttpService {
    return HttpService { ctx, req ->
        HttpResponse.of(CoroutineScope(ctx.asCoroutineContext()).future {
            serve(ctx, req)
        })
    }
}

Ditto for AbstractHttpService.

Inheritance

Pros:

  • Drop-in replacement to HttpService because it inherits.

Cons:

  • Function name needs to be different such as serveAsync or coServe.
  • Needs to be an abstract class to prevent overriding serve.

Composition

Pros:

  • Method signatures are the same
  • Interface, similar to HttpService.

Cons:

  • Need to use .asHttpService() method to convert to an HttpService

Also need to check if we lose any valuable method if we don't inherit HttpService.

@trustin trustin changed the title Support coroutines in HttpService. Provide an easy way to create an HttpService from a Kotlin coroutine Mar 11, 2024
@minwoox
Copy link
Member

minwoox commented Mar 27, 2024

Thanks, @Dogacel for the design. 👍

Needs to be an abstract class to prevent overriding serve.

I believe that we don't have to make it an abstract class as we do for CoroutineServerInterceptor:

Because users don't have to call .asHttpService() when using it, I prefer the first option.
Here is the interface version.
I also rename the method to suspendedServe. 😉

interface CoroutineHttpService : HttpService {

    override fun serve(ctx: ServiceRequestContext, req: HttpRequest): HttpResponse {
        return HttpResponse.of(CoroutineScope(ctx.asCoroutineContext()).future { // Do we also need to add user context?
            suspendedServe(ctx, req)
        })
    }

    suspend fun suspendedServe(ctx: ServiceRequestContext, req: HttpRequest): HttpResponse
}

@Dogacel
Copy link
Contributor Author

Dogacel commented Mar 27, 2024

Thanks, @Dogacel for the design. 👍

Needs to be an abstract class to prevent overriding serve.

I believe that we don't have to make it an abstract class as we do for CoroutineServerInterceptor:

If this risk is something accepted down the line, I'm fine with it 😄.

Because users don't have to call .asHttpService() when using it, I prefer the first option. Here is the interface version. I also rename the method to suspendedServe. 😉

interface CoroutineHttpService : HttpService {

    override fun serve(ctx: ServiceRequestContext, req: HttpRequest): HttpResponse {
        return HttpResponse.of(CoroutineScope(ctx.asCoroutineContext()).future { // Do we also need to add user context?
            suspendedServe(ctx, req)
        })
    }

    suspend fun suspendedServe(ctx: ServiceRequestContext, req: HttpRequest): HttpResponse
}

LGTM 👍 Consistency is desired.

@minwoox minwoox added the sprint Issues for OSS Sprint participants label Mar 28, 2024
@minwoox
Copy link
Member

minwoox commented Mar 28, 2024

Thanks! By the way, this issue has been assigned to a participant for our internal sprint. 😉

minwoox added a commit that referenced this issue May 10, 2024
…5603)

Motivation:
- Closes #5442

Modifications:
- Add CoroutineHttpService implements HttpService

Result:
- Closes #5442.
- You can now use `CoroutineHttpService` that runs your service in a coroutine scope.
Co-authored-by: minwoox <songmw725@gmail.com>
Dogacel pushed a commit to Dogacel/armeria that referenced this issue 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
Labels
new feature sprint Issues for OSS Sprint participants
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants