swagger-api / swagger-codegen

swagger-codegen contains a template-driven engine to generate documentation, API clients and server stubs in different languages by parsing your OpenAPI / Swagger definition.
http://swagger.io
Apache License 2.0
16.9k stars 6.03k forks source link

[Java-Playframework] Blocking APIs #6182

Open tzimisce012 opened 7 years ago

tzimisce012 commented 7 years ago
Description

If the api is making a blocking operation, the controller should return a CompletationStage

Swagger-codegen version

2.3.0

Steps to reproduce

In the newApiController.mustache all ApiActions methods should return CompletationStage

Suggest a fix/enhancement

I would make all methods return CompletationStage. If the operation is synchronous we can just make CompletableFuture.completedFuture

JFCote commented 7 years ago

@tzimisce012 I've checked the latest documentation and examples for Play 2.6 and just as I expected, async controller is not de default.

See examples here: https://github.com/playframework/play-java-starter-example/tree/2.6.x/app/controllers

I think that, like in other generators, we should add this as an option that is false by default. The option should be called "async", like in the Spring generator: https://github.com/swagger-api/swagger-codegen/blob/master/modules/swagger-codegen/src/main/java/io/swagger/codegen/languages/SpringCodegen.java#L75

I propose that you close PR #6186 since you are the creator and create a new PR with the new option to use async call.

Thanks!

tzimisce012 commented 7 years ago

It is not the default option in the sense that play start explaining you how the framework works not talking about the fact that your operation might be blocking.

But returning CompletationStage is more general than just returning Result in the sense that all code that will work with Result will work with CompletationStage but the opposite will not be true.

I consider that setting the most general behavior is better since we do not know if the api services will be blocking or not. In case they are blocking (this will be actually quite common), the controllers will need to be changed.

But anyways, i could add another tag to just give the option to the developer to set async or not is controller, but I still think it is better idea to put the aync behaviour as default...

JFCote commented 7 years ago

@tzimisce012 I understand your point but all the other generator in swagger-codegen are blocking by default and users that want async controllers will have the options (if you submit a PR with the feature). I find that putting CompletionStage by default when no async is needed is just complex for nothing when a simple Result do the job. Also, most of the users will be more than happy with a blocking controller. The day they decide to switch to async, they would change the variable "async" in their generation and 1 second later they have all of their function async :)

So, if you have time, please submit a PR with this feature (async CLI options + mustache modification) and I'll review it.

Again, thanks for your feedback, it is really appreciated!

tzimisce012 commented 7 years ago

I mean, the other generators are bloking because they have another thread model. In play, if you have a blocking operation, play just stop working.

In spring, you have the option to have the classical thread per request or you can use Spring Reactor that implements the Reactor pattern well known by NodeJs. In that case, your code must be async otherwise your event loop thread will be blocked and the server will be unresponsive.

That is what happens with play, but play does not have the options. Play is async by default, it does not have the option of using the thread per request model.

Anyways I will add this as an optional feature.

JFCote commented 6 years ago

@tzimisce012 Any progress on this feature?

tzimisce012 commented 6 years ago

I have been quite busy in my job but I have it on my TODOs. I have the hope of doing this on christmas holidays

JFCote commented 6 years ago

Thanks for the update!