StripesFramework / stripes

Stripes is a Java framework with the goal of making Servlet/JSP based web development in Java as easy, intuitive and straight-forward as it should be. It's stripey and it doesn't suck.
http://www.stripesframework.org/
171 stars 73 forks source link

support for async action beans #37

Open vankeisb opened 8 years ago

vankeisb commented 8 years ago

In some cases (e.g. ActionBean calling webservice(s) or async messaging), we'd like to use the Servlet3 async feature, that allows for better performance (non-blocking).

Stripes could provide some plumbing in order to make it simple, and integrated with the rest of the framework.

iluvtr commented 8 years ago

That would be really nice!

kfowlks commented 8 years ago

+1 On this feature

timothystone commented 8 years ago

+1

vankeisb commented 8 years ago

Hiya,

I have committed a first draft of Async Stripes. Feedback most appreciated.

The idea is to allow ActionBean implementor to return instances of AsyncResolution, an abstract class that Stripes provides and which allows for Servlet3 async processing.

When an action's event handler returns AsyncResolution, then Stripes triggers the async mode (via request.startAsync()), and lets the user complete the request.

Here's an example of such an action, that uses a non-blocking http client to fetch data from a remote service, and completes the request via a callback :

https://github.com/StripesFramework/stripes/blob/feature/async/examples/src/main/java/net/sourceforge/stripes/examples/async/AsyncActionBean.java#L52

I had to change the lifecycle a bit (only for async : sync actions should be impacted), and to implement new interface methods here and there (like layouts etc) in order to support servlet 3. So it has to be extensively tested : servlet3 is a big move. Bigger than async actually...

Also, there ain't no support for async stuff in MockRoundtrip at the moment.

I have pushed a version to maven central so that you don't have to recompile for testing :

<dependency>
    <groupId>net.sourceforge.stripes</groupId>
    <artifactId>stripes</artifactId>
    <version>1.7.0-async-beta</version>
</dependency>

It's pushed in branch feature/async.

Please let me know if you have bugs, or see anything suspicious.

iluvtr commented 8 years ago

Hi, Remi, It' a interesting move going to Servlet 3.0. I checked out your sample, I think it will better to create a new annotation @Async than introducing that AsyncResolution class. This annotation enables async behaviour to any existing Resolution. Currently Spring MVC and JAX RS 2 works this way. Like this:

@Async public Resolution executeLongTask(){ customers = someService.getAllCustomers(); return new ForwardResolution(VIEW); } El 23/12/2015 5:28 a. m., "Remi Vankeisbelck" notifications@github.com escribió:

Hiya,

I have committed a first draft of Async Stripes. Feedback most appreciated.

The idea is to allow ActionBean implementor to return instances of AsyncResolution, an abstract class that Stripes provides and which allows for Servlet3 async processing.

When an action's event handler returns AsyncResolution, then Stripes triggers the async mode (via request.startAsync()), and lets the user complete the request.

Here's an example of such an action, that uses a non-blocking http client to fetch data from a remote service, and completes the request via a callback :

https://github.com/StripesFramework/stripes/blob/feature/async/examples/src/main/java/net/sourceforge/stripes/examples/async/AsyncActionBean.java#L52

I had to change the lifecycle a bit (only for async : sync actions should be impacted), and to implement new interface methods here and there (like layouts etc) in order to support servlet 3. So it has to be extensively tested : servlet3 is a big move. Bigger th

Also, there ain't no support for async stuff in MockRoundtrip at the moment.

I have pushed a version to maven central so that you don't have to recompile for testing :

net.sourceforge.stripes stripes 1.7.0-async-beta

It's pushed in branch feature/async.

Please let me know if you have bugs, or see anything suspicious.

— Reply to this email directly or view it on GitHub https://github.com/StripesFramework/stripes/issues/37#issuecomment-166856853 .

vankeisb commented 8 years ago

Hi,

Spring's @Async is something totally different : it's meant to have component methods execute in a separate thread (with an executor), and has nothing to do with Servlet 3 AFAIK. Plus you'd have to return a Future<Resolution> in that case.

Spring uses Callable in order to achieve what I'm trying to do with Stripes : http://shengwangi.blogspot.fr/2015/09/asynchronous-spring-mvc-hello-world.html

I prefer the base class. It's simple, easy, and effective. Plus I fail to see how you get access to the AsyncContext with the annotation (and even with the Callable approach)...

Cheers

Rémi

vankeisb commented 8 years ago

Problem with tomcat7 :

A filter or servlet of the current chain does not support asynchronous operations.

Seems related to DMF. Because it creates an "internal" instance of the StripesDispatcher servlet, and tomcat ain't happy with this.

iluvtr commented 8 years ago

Hi Remi, my proposal of an @Async annotation is independent from Spring. Here is the idea:

Normally we have action methods that returns a Resolution, something like this:

public Resolution someWork() { someService.doSomeHardWork(); //Finally! return new ForwardResolution(VIEW); }

As you can see the hard work is encapsulated in the method, this could be executed by a tuned Java ExecutorService. The returned resolution could be executed as async return. In fact you don't need to call AsyncContext at all, the call to the method asyncContext.dispatch() could be integrated in Stripes' ForwardResolution. We could improve StreamingResolution or other Stripes resolutions to call asyncContext.complete() at the end of the method.

The process could be like this:

Your example could be really simplified: You won't need any CloseableHttpAsyncClient, just a normal HttpClient, because the long process is executed in other thread, but at the end of the method we commit a normal servlet response. Check out:

//When the action method is @Async annotated, the method is executed in another //thread managed by an ExecutorService @Async public Resolution asyncEvent() { //A normal Http client, but this will executed in another thread. HttpClient client = HttpClientBuilder.create().build(); HttpPost post = new HttpPost(url); HttpResponse result = client.execute(post); int status = result.getStatusLine().getStatusCode(); return new ForwardResolution("/WEB-INF/async/async.jsp"); } Internally in the code of ForwardResolution method .... log.trace("Forwarding to URL: ", path); if(request.isAsyncStarted()) { AsyncContext asyncContext = request.getAsyncContext(); asyncContext.dispatch(path); } else { request.getRequestDispatcher(path).forward(request, response); }

Here link showing ExecutorService in action:

http://www.javacodegeeks.com/2013/08/async-servlet-feature-of-servlet-3.html http://www.javaworld.com/article/2077995/java-concurrency/asynchronous-processing-support-in-servlet-3-0.html

2015-12-24 4:40 GMT-05:00 Remi Vankeisbelck notifications@github.com:

Hi,

Spring's @Async https://github.com/Async is something totally different : it's meant to have component methods execute in a separate thread (with an executor), and has nothing to do with Servlet 3 AFAIK. Plus you'd have to return a Future in that case.

Spring uses Callable in order to achieve what I'm trying to do with Stripes :

http://shengwangi.blogspot.fr/2015/09/asynchronous-spring-mvc-hello-world.html

I prefer the base class. It's simple, easy, and effective. Plus I fail to see how you get access to the AsyncContext with the annotation (and even with the Callable approach)...

Cheers

Rémi

— Reply to this email directly or view it on GitHub https://github.com/StripesFramework/stripes/issues/37#issuecomment-167078184 .

vankeisb commented 8 years ago

I like the idea of reusing ForwardResolution etc. I fully agree that the current impl is a draft and lacks higher level API for complete/dispatch. I was thinking of helper methods on the base AsyncResolution class. But it would be nicer to be able to reuse existing Resolutions if possible and meaningful.

Otoh, the ExecutorService and non-blocking http client is a completely different matter. In my sample you precisely don't want another thread pool... What you want is to avoid blocking the appserver's threads while performing I/O operations... You cannot do this with a blocking client in an executor service : you'd end up starving this pool instead of the AS's.

One should not confuse long-running operations with non blocking I/O. Those are 2 completely different things.

Le 31 déc. 2015 à 03:43, iluvtr notifications@github.com a écrit :

Hi Remi, my proposal of an @Async annotation is independent from Spring. Here is the idea:

Normally we have action methods that returns a Resolution, something like this:

public Resolution someWork() { someService.doSomeHardWork(); //Finally! return new ForwardResolution(VIEW); }

As you can see the hard work is encapsulated in the method, this could be executed by a tuned Java ExecutorService. The returned resolution could be executed as async return. In fact you don't need to call AsyncContext at all, the call to the method asyncContext.dispatch() could be integrated in Stripes' ForwardResolution. We could improve StreamingResolution or other Stripes resolutions to call asyncContext.complete() at the end of the method.

The process could be like this:

  • Stripes executes an action method that return a Resolution in an ExecutorService
  • Stripes, at the end of the method, calls asyncContext. dispatch() or asyncContext.complete()

Your example could be really simplified: You won't need any CloseableHttpAsyncClient, just a normal HttpClient, because the long process is executed in other thread, but at the end of the method we commit a normal servlet response. Check out:

//When the action method is @Async annotated, the method is executed in another //thread managed by an ExecutorService @Async public Resolution asyncEvent() { //A normal Http client, but this will executed in another thread. HttpClient client = HttpClientBuilder.create().build(); HttpPost post = new HttpPost(url); HttpResponse result = client.execute(post); int status = result.getStatusLine().getStatusCode(); return new ForwardResolution("/WEB-INF/async/async.jsp"); } Internally in the code of ForwardResolution method .... log.trace("Forwarding to URL: ", path); if(request.isAsyncStarted()) { AsyncContext asyncContext = request.getAsyncContext(); asyncContext.dispatch(path); } else { request.getRequestDispatcher(path).forward(request, response); }

Here link showing ExecutorService in action:

http://www.javacodegeeks.com/2013/08/async-servlet-feature-of-servlet-3.html http://www.javaworld.com/article/2077995/java-concurrency/asynchronous-processing-support-in-servlet-3-0.html

2015-12-24 4:40 GMT-05:00 Remi Vankeisbelck notifications@github.com:

Hi,

Spring's @Async https://github.com/Async is something totally different : it's meant to have component methods execute in a separate thread (with an executor), and has nothing to do with Servlet 3 AFAIK. Plus you'd have to return a Future in that case.

Spring uses Callable in order to achieve what I'm trying to do with Stripes :

http://shengwangi.blogspot.fr/2015/09/asynchronous-spring-mvc-hello-world.html

I prefer the base class. It's simple, easy, and effective. Plus I fail to see how you get access to the AsyncContext with the annotation (and even with the Callable approach)...

Cheers

Rémi

— Reply to this email directly or view it on GitHub https://github.com/StripesFramework/stripes/issues/37#issuecomment-167078184 .

— Reply to this email directly or view it on GitHub.

vankeisb commented 8 years ago

Ok, I have a better version now, with MockRoundtrip working (ie you can unit test async actions...).

@iluvtr I've thought about your comments on reusing Resolutions, and I think you are right. Dispatch, for example, is an easy one : it could reuse the Stripes APIs like you describe in ForwardResolution. My main concern is to know if it applies to all kinds of resolutions. Async writes can be different of regular, sync writes, so we could have incompatibilities there. I'm gonna continue digging...

I'm still not sold on the annotation though. I think we need a callback mechanism, which will probably be pretty clumsy with an annotation.

So if we can reuse the base Resolution interface, a possible way to do this "cleaner" would be to use a Consumer<Resolution> as an arg of the event method.

For example, this regular, blocking event method :

public void doStuff(ActionBeanContext abc) {
  doSomeStuff();
  return new ForwardResolution("/foo.jsp");
}

Could become asynchronous by writing it like this :

public Resolution doStuff(ActionBeanContext abc, Consumer<Resolution> callback) {
  doSomeStuff();
  callback.accept(new ForwardResolution("/foo.jsp"));
}

Stripes could then detect async event handlers by looking at the method signature (it already does), and find the callback param. It would then use the code that currently checks if the returned Resolution is an async one or not.

I think it's better than the current AsyncResolution approach. Any comments ?

iluvtr commented 8 years ago

Hi Remi, your current way is much better. It reminds me JAXRS' AsyncResponse https://jersey.java.net/documentation/latest/async.html. Check it out. There's no need to pass ActionBeanContext as argument, it would be enough to pass the object to resume the process.

Cheers. El 18/1/2016 8:15 a. m., "Remi Vankeisbelck" notifications@github.com escribió:

Ok, I have a better version now, with MockRoundtrip working (ie you can unit test async actions...).

@iluvtr https://github.com/iluvtr I've thought about your comments on reusing Resolutions, and I think you are right. Dispatch, for example, is an easy one : it could reuse the Stripes APIs like you describe in ForwardResolution. My main concern is to know if it applies to all kinds of resolutions. Async writes can be different of regular, sync writes, so we could have incompatibilities there. I'm gonna continue digging...

I'm still not sold on the annotation though. I think we need a callback mechanism, which will probably be pretty clumsy with an annotation.

So if we can reuse the base Resolution interface, a possible way to do this "cleaner" would be to use a Consumer as an arg of the event method.

For example, this regular, blocking event method :

public void doStuff(ActionBeanContext abc) { doSomeStuff(); return new ForwardResolution("/foo.jsp"); }

Could become asynchronous by writing it like this :

public Resolution doStuff(ActionBeanContext abc, Consumer callback) { doSomeStuff(); callback.accept(new ForwardResolution("/foo.jsp")); }

Stripes could then detect async event handlers by looking at the method signature (it already does), and find the callback param. It would then use the code that currently checks if the returned Resolution is an async one or not.

I think it's better than the current AsyncResolution approach. Any comments ?

— Reply to this email directly or view it on GitHub https://github.com/StripesFramework/stripes/issues/37#issuecomment-172524327 .

vankeisb commented 8 years ago

Yep it's the regular "callback" approach, I also think it's better.

The ActionBeanContext arg is optional in Stripes event handlers : you may declare it or not. So the only "requirement" to async handlers is that :

Unfortunately it's harder to implement against the current codebase... Requires some pretty heavy surgery in DispatcherHelper/Servlet : they were designed for synchronous event handlers. Patching Resolution subclasses should be OK but a good portion of the dispatch code needs to be refactored.

@rgrashel : I thought about this : is the REST code needed in "core" dispatcher ? can't this be handled via interceptors (and therefore be less intrusive) ?

iluvtr commented 8 years ago

Remi, I think we should think a better name instead of 'Consumer' because of Java 8 Consumer in package java.util El 18/1/2016 8:33 a. m., "Remi Vankeisbelck" notifications@github.com escribió:

Yep it's the regular "callback" approach, I also think it's better.

The ActionBeanContext arg is optional in Stripes event handlers : you may declare it or not. So the only "requirement" to async handlers is that :

  • it returns void
  • it has one and only one arg of type Consumer

Unfortunately it's harder to implement against the current codebase... Requires some pretty heavy surgery in DispatcherHelper/Servlet : they were designed for synchronous event handlers. Patching Resolution subclasses should be OK but a good portion of the dispatch code needs to be refactored.

@rgrashel https://github.com/rgrashel : I thought about this : is the REST code needed in "core" dispatcher ? can't this be handled via interceptors (and therefore be less intrusive) ?

— Reply to this email directly or view it on GitHub https://github.com/StripesFramework/stripes/issues/37#issuecomment-172528070 .

vankeisb commented 8 years ago

It's the one from Java8 I'm talking about :)

vankeisb commented 8 years ago

No need for another @FunctionalInterface here

iluvtr commented 8 years ago

Oh, Stripes currently is supporting Java >= 5. I think it's a bold move to directly require Java 8. Why not start for requiring Java 7? El 18/1/2016 9:04 a. m., "Remi Vankeisbelck" notifications@github.com escribió:

No need for another @FunctionalInterface https://github.com/FunctionalInterface here

— Reply to this email directly or view it on GitHub https://github.com/StripesFramework/stripes/issues/37#issuecomment-172534122 .

vankeisb commented 8 years ago

Java7 was eol-ed last year. I'd say we go for Java8 directly... Any objection ?

rgrashel commented 8 years ago

The reason I couldn't use interceptors for REST action beans is because of some slightly different behavior that needs to happen with regards to exception handling and a couple of other things. Those other things are not exposed to interceptors.

That's not to criticize interceptors or dispatching, but it would have been almost impossible for the original authors to dream up use cases that would be needed 8 years later.

If we were doing it "right", I think the best move would be to make the entire Stripes life cycle pluggable. But I am not sure that is the most practical approach given the maturity of the project.

-- Rick On Jan 18, 2016 7:33 AM, "Remi Vankeisbelck" notifications@github.com wrote:

Yep it's the regular "callback" approach, I also think it's better.

The ActionBeanContext arg is optional in Stripes event handlers : you may declare it or not. So the only "requirement" to async handlers is that :

  • it returns void
  • it has one and only one arg of type Consumer

Unfortunately it's harder to implement against the current codebase... Requires some pretty heavy surgery in DispatcherHelper/Servlet : they were designed for synchronous event handlers. Patching Resolution subclasses should be OK but a good portion of the dispatch code needs to be refactored.

@rgrashel https://github.com/rgrashel : I thought about this : is the REST code needed in "core" dispatcher ? can't this be handled via interceptors (and therefore be less intrusive) ?

— Reply to this email directly or view it on GitHub https://github.com/StripesFramework/stripes/issues/37#issuecomment-172528070 .

vankeisb commented 8 years ago

@rgrashel thanks for the explanation. And I totally agree with you about the design of interceptors and the whole dispatch chain. I also fully agree with the re-write of the whole thing. I've faced issues that I had to workaround because of the current design. It's also a "problem" for the async thing. But I also think it's unrealistic to do this now, and I'm unsure of the short-term benefits.

If we manage to go Servlet3 and have async, I think it's good enough for the upcoming major release.

We have to see for Java 8. I'm using it but it's no blocker, I can rewrite in Java7 (or even 5 maybe). It's a group decision...

roncking commented 8 years ago

I've been working on upgrading a Stripes, jax-ws based client to be async. I was wondering what would be the best way to 'abandon' an ActionBean that no longer needs to return a Resolution. I understand that the solution this thread is talking about will be better, but I've a short term need to make a web service call from Stripes async as soon as possible. Does Stripes have servlet.destroy() ? I know it's crude, but what about throwing some sort of exception as an option?

rgrashel commented 8 years ago

I know a lot of people just implement Resolution their own and create a "NullResolution". All the execute() method does with these things is set some sort of status code on the response. That should be all that is necessary. Something like this.

public class NullResolution implements Resolution { public void execute( HttpServletRequest request, HttpServletResponse response ) { response.setStatus( HttpServletResponse.SC_OK ); } }

Or something like that. So create a null resolution and return that at the end of your asynchronous call. Inside of your action method, you could kick off a worker which does the work and then return the NullResolution. That should be all you need to do.

-- Rick

On Thu, Jan 21, 2016 at 1:45 PM, roncking notifications@github.com wrote:

I've been working on upgrading a Stripes, jax-ws based client to be async. I was wondering what would be the best way to 'abandon' an ActionBean that no longer needs to return a Resolution. I understand that the solution this thread is talking about will be better, but I've a short term need to make a web service call from Stripes async as soon as possible. Does Stripes have servlet.destroy() ? I know it's crude, but what about throwing some sort of exception as an option?

— Reply to this email directly or view it on GitHub https://github.com/StripesFramework/stripes/issues/37#issuecomment-173687756 .

roncking commented 8 years ago

Thanks! That should solve my problem.

On Thu, Jan 21, 2016 at 2:18 PM, Rick Grashel notifications@github.com wrote:

I know a lot of people just implement Resolution their own "NullResolution". All the execute() method does with these things is set some sort of status code on the request. That should be all that is necessary. Something like this.

public class NullResolution implements Resolution { public void execute( HttpServletRequest request, HttpServletResponse response ) { response.setStatus( HttpServletResponse.SC_OK ); } }

Or something like that. So create a null resolution and return that at the end of your asynchronous call. That should be all you need to do.

-- Rick

On Thu, Jan 21, 2016 at 1:45 PM, roncking notifications@github.com wrote:

I've been working on upgrading a Stripes, jax-ws based client to be async. I was wondering what would be the best way to 'abandon' an ActionBean that no longer needs to return a Resolution. I understand that the solution this thread is talking about will be better, but I've a short term need to make a web service call from Stripes async as soon as possible. Does Stripes have servlet.destroy() ? I know it's crude, but what about throwing some sort of exception as an option?

— Reply to this email directly or view it on GitHub < https://github.com/StripesFramework/stripes/issues/37#issuecomment-173687756

.

— Reply to this email directly or view it on GitHub https://github.com/StripesFramework/stripes/issues/37#issuecomment-173696007 .

vankeisb commented 8 years ago

I don't really understand how this could work... If you use an asynchronous client, by definition, the Stripes event handler will end before your client has done its job. So this means that you will not be able to use the response of the webservice you are invoking from the action bean, you'll always return the same Resolution, no matter what. I wonder how this can be of any use : it's basically a "blind" controller that calls web services but always responds the same thing...

vankeisb commented 8 years ago

@roncking I'm afraid you'll have to wait for the async feature if you really want to use an async client in an efficient manner (ie using callbacks).

Otherwise, you need to block yourself with your async client. This is usually done by calling Future.get() on modern async clients. Note that this defeats the purpose for async clients, as blocking in the action bean is strictly equivalent to blocking in the http client... But that's the only way to do it with Stripes currently, without async servlets.

vankeisb commented 8 years ago

Ok, I have made some progress. It's not final, but it's getting better.

I'm still using the AsyncResolution base class, but I have added APIs for completing with existing Resolution classes, which makes it more "Stripey".

For example :

public Resolution doItAsynchronous() {
    return new AsyncResolution() {
        @Override
        protected void executeAsync() throws Exception {
            // complete with a good old Stripes Resolution : this is 
            // done in response to a callback or other non blocking call...
            complete(new ForwardResolution("/foo/bar.jsp"));
        }
    };
}

There's still lots of boilerplate. I'm trying to find a way to make this better (the Consumer<Resolution> thing...) but I think I'll have to change the dispatching code a little bit in order to support that.

I also have modified MockRoundtrip so that async actions work in unit tests. I do not guarantee that this will behave like a regular appserver, but async actions can be tested transparently : trip.execute() blocks just like for a "regular" action until the async action completes/dispatches.

Feedback / review most appreciated.

roncking commented 8 years ago

Here's what I'm doing, and it's not quite working. When the user presses submit at he beginning of the ActionBean I use the response writer to write a message to the user, telling them that they will receive an email with the transaction number in a few minutes. I'm on Tomcat 8, and even if I flush the writers buffer, the message doesn't show up until after I return the NullResolution about a minute later. The message is a simple html page, with two buttons, one to continue working on the website, and one for if the user is 'done'. The web service returns a result in about a minute, and I'm able to send an email to the user then, so the user doesn't have to wait on the service to complete. I'm trying to decouple the web service transaction time from the user experience, does this seem reasonable?

Should I be trying something different? Is this something that I should be using Ajax for?

On Sat, Jan 23, 2016 at 7:00 AM, Remi Vankeisbelck <notifications@github.com

wrote:

I don't really understand how this could work... If you use an asynchronous client, by definition, the Stripes event handler will end before your client has done its job. So this means that you will not be able to use the response of the webservice you are invoking from the action bean, you'll always return the same Resolution, no matter what. I wonder how this can be of any use : it's basically a "blind" controller that calls web services but always responds the same thing...

— Reply to this email directly or view it on GitHub https://github.com/StripesFramework/stripes/issues/37#issuecomment-174184879 .

rgrashel commented 8 years ago

This sort of gets into what your application needs to do and the architecture.

If this is a transaction of some sort, then it sounds like this needs to be durable. In this case, what I would do is have the Stripes action, perform whatever validation is necessary, package up the information from the form and insert it into a database table. Then, I would create some sort of recurring job (like an EJB Timer or a Quartz job) that runs inside of the application server. Maybe it polls every minute (or whatever time frame makes sense). It looks in this database table for incoming transaction information that needs to be processed.

Based on your post, it sounds to me like what you need is an asynchronous process, but not an asynchronous client.

-- Rick

On Sat, Jan 23, 2016 at 7:54 AM, roncking notifications@github.com wrote:

Here's what I'm doing, and it's not quite working. When the user presses submit at he beginning of the ActionBean I use the response writer to write a message to the user, telling them that they will receive an email with the transaction number in a few minutes. I'm on Tomcat 8, and even if I flush the writers buffer, the message doesn't show up until after I return the NullResolution about a minute later. The message is a simple html page, with two buttons, one to continue working on the website, and one for if the user is 'done'. The web service returns a result in about a minute, and I'm able to send an email to the user then, so the user doesn't have to wait on the service to complete. I'm trying to decouple the web service transaction time from the user experience, does this seem reasonable?

Should I be trying something different? Is this something that I should be using Ajax for?

On Sat, Jan 23, 2016 at 7:00 AM, Remi Vankeisbelck < notifications@github.com

wrote:

I don't really understand how this could work... If you use an asynchronous client, by definition, the Stripes event handler will end before your client has done its job. So this means that you will not be able to use the response of the webservice you are invoking from the action bean, you'll always return the same Resolution, no matter what. I wonder how this can be of any use : it's basically a "blind" controller that calls web services but always responds the same thing...

— Reply to this email directly or view it on GitHub < https://github.com/StripesFramework/stripes/issues/37#issuecomment-174184879

.

— Reply to this email directly or view it on GitHub https://github.com/StripesFramework/stripes/issues/37#issuecomment-174187991 .

roncking commented 8 years ago

The Stripes application is communicating with a web service that I don't have much control over, and I think what you're describing that's doing the polling is the web service, correct? I don't need 100% durability, 99% is fine ;-)

Can you tell me how to get the Writer output to be flushed to the web browser instantly? If I get that working, I think I have a workable solution by using email to send the transaction status to the user.

rgrashel commented 8 years ago

What application server are you using?

On Sat, Jan 23, 2016 at 8:54 AM, roncking notifications@github.com wrote:

The Stripes application is communicating with a web service that I don't have much control over, and I think what you're describing that's doing the polling is the web service, correct? I don't need 100% durability, 99% is fine ;-)

Can you tell me how to get the Writer output to be flushed to the web browser instantly? If I get that working, I think I have a workable solution by using email to send the transaction status to the user.

— Reply to this email directly or view it on GitHub https://github.com/StripesFramework/stripes/issues/37#issuecomment-174191914 .

vankeisb commented 8 years ago

I think Rick is right. You need a background scheduler, and probably polling, but this has nothing to do with non-blocking controllers, which is the subject of this ticket.

@roncking Do you have access to the Mailing List ? We can help you with your problem there, we all have done this I guess (spawning long running tasks in the background). I'd prefer not to "pollute" this ticket, as it's not the topic. You can also open another issue in github.

roncking commented 8 years ago

I'm using Tomcat 8.

I'm sorry that I polluted this thread, how do I find the Stripes mailing list? I'll ask my question there instead. Thanks!

On Sat, Jan 23, 2016 at 9:06 AM, Remi Vankeisbelck <notifications@github.com

wrote:

I think Rick is right. You need a background scheduler, and probably polling, but this has nothing to do with non-blocking controllers, which is the subject of this ticket.

@roncking https://github.com/roncking Do you have access to the Mailing List ? We can help you with your problem there, we all have done this I guess (spawning long running tasks in the background). I'd prefer not to "pollute" this ticket, as it's not the topic. You can also open another issue in github.

— Reply to this email directly or view it on GitHub https://github.com/StripesFramework/stripes/issues/37#issuecomment-174192450 .

vankeisb commented 8 years ago

Ok, I've refactored to something much better I think. Here's what the async handler looks like now :

public void asyncHandler(AsyncResolution async) {
    // do whatever you need, can be non blocking, and
    // don't forget to call complete()...
    async.complete(new ForwardResolution("/foo/bar"));
}

Of course, it's meant to be used with callback-based APIs, or in separate threads. Simplistic example :

public void asyncHandler(AsyncResolution async) {
    new Thread(() -> {
        // do stuff in separate thread and complete...
        async.complete(new ForwardResolution("/foo/bar"));
    }).start();
}

As you can see, the AsyncResolution is not used for subclassing any more. You just call methods on it in order to complete the async processing.

The method signature tells Stripes it's async because :

When Stripes encounters such an "async handler", it starts an asynchronous cycle on the http request for you, and handles cleanup etc.

Only event handling can be async : binding, interceptors are still synchronous.

vankeisb commented 8 years ago

Hi folks,

I have refactored some stuff in order to cut the runtime dependency on Servlet3 APIs. Stripes continues to run as usual on Servlet2+ containers (ie it still runs in old tomcat6 etc).

Async events can be used in any Servlet3+ container (tomcat7+, jetty8+, etc.).

I've also added profiles to the pom, in order to ease integration testing : they allow to run the tests on jetty92, tomcat7 and tomcat6.

mvn clean install -Pwebtests,jetty92 -Dwebtests.browser=chrome -Dwebdriver.chrome.driver=/path/to/chromedriver
mvn clean install -Pwebtests,tomcat7 -Dwebtests.browser=chrome -Dwebdriver.chrome.driver=/path/to/chromedriver
mvn clean install -Pwebtests,tomcat6 -Dwebtests.browser=chrome -Dwebdriver.chrome.driver=/path/to/chromedriver

The async webtest fails on tomcat6, which is expected.

The tests also show a bug with tomcat7 (mentioned above in this thread), which we need to fix before we can "ship" the feature.

The API should be stable, so I think it's time for more testing. So, if you want to help, please try the Async events in you app(s).

An easy way to test is to "morph" some of your actual event handlers so that they do the same thing than before, but in "asynchronous style" :

public Resolution foo() {
    ...
    return new ForwardResolution("/bar.jsp");
}

becomes :

public void foo(AsyncResolution async) {
    ...
    async.complete(new ForwardResolution("/bar.jsp"));
}

Even if it does nothing fancy, it'll use all the async dispatch code and will help spotting bugs, if any.

We now need more testing, and feedback. Async actions heavily rely on container internals, and therefore the most containers we test, the better.

I haven't published anything to maven central yet, so please checkout the feature/async branch, and build stripes locally (mvn clean install). Then you need to depend onthe 1.7.0-async-SNAPSHOT version.

iluvtr commented 8 years ago

Hi Remi, The name 'AsyncResolution' is a bit confusing because its purpose differs from normal Resolutions. How about 'AsyncResponse'?

2016-01-27 8:52 GMT-05:00 Remi Vankeisbelck notifications@github.com:

Hi folks,

I have refactored some stuff in order to cut the compile-time dependency on Servlet3 APIs. Stripes continues to run as usual on Servlet2+ containers (ie it still runs in old tomcat6 etc).

Async events can be used in any Servlet3+ container (tomcat7+, jetty8+, etc.).

I've also added profiles to the pom, in order to ease integration testing : they allow to run the tests on jetty92, tomcat7 and tomcat6.

mvn clean install -Pwebtests,jetty92 -Dwebtests.browser=chrome -Dwebdriver.chrome.driver=/path/to/chromedriver mvn clean install -Pwebtests,tomcat7 -Dwebtests.browser=chrome -Dwebdriver.chrome.driver=/path/to/chromedriver mvn clean install -Pwebtests,tomcat6 -Dwebtests.browser=chrome -Dwebdriver.chrome.driver=/path/to/chromedriver

The async webtest fails on tomcat6, which is expected.

The tests also show a bug with tomcat7 (mentioned above in this thread), which we need to fix before we can "ship" the feature.

The API should be stable, so I think it's time for more testing. So, if you want to help, please try the Async events in you app(s).

An easy way to test is to "morph" some of your actual event handlers so that they do the same thing than before, but in "asynchronous style" :

public Resolution foo() { ... return new ForwardResolution("/bar.jsp"); }

becomes :

public void foo(AsyncResolution async) { ... async.complete(new ForwardResolution("/bar.jsp")); }

Even if it does nothing fancy, it'll use all the async dispatch code and will help spotting bugs, if any.

We now need more testing, and feedback. Async actions heavily rely on container internals, and therefore the most containers we test, the better.

I haven't published anything to maven central yet, so please checkout the feature/async branch, and build stripes locally (mvn clean install). Then you need to depend onthe 1.7.0-async-SNAPSHOT version.

— Reply to this email directly or view it on GitHub https://github.com/StripesFramework/stripes/issues/37#issuecomment-175636628 .

vankeisb commented 8 years ago

Agreed, bad naming. I like AsyncResponse. I'm refactoring this right now.

rgrashel commented 8 years ago

I've been looking at the AsyncActionBean.java example. I honestly think we should push this entire asychronous execution logic into the dispatcher. In the AsyncActionBean example, all it ultimately does it return a resolution. The only differentiator between this and any other event handler is that the desire is for the event to run asychronously. An annotation could solve this nicely. For example, the event in AsyncActionBean.java could be written as:

@Asynchronous
public Resolution asyncEvent() {
    // Put code here that calls the Github service
    return new ForwardResolution( JSP_PATH );
}

This method could just as easily return a streaming resolution of JSON or any kind of other resolution. I also think doing this using an annotation would be much more in line with how this is done in JEE containers. I'm not in the middle of this, so I definitely might be missing something.

vankeisb commented 8 years ago

@rgrashel An annotation doesn't do the job. You need to pass a callback arg.

The whole point of non blocking actions is that you leave the request thread when starting an async cycle.

For example, assume you launch a background task (submit a Runnable to a pool) from the action and want to complete() at the end ?

public Resolution impossibleAsync() {
  executor.submit(new Runnable() {
    public void run() {
      // decide here which Resolution to return depending on the biz logic
      // but we're async, and Runnables do not return anything :(
    }
   });
   return ??? ;
}

See ? Has to be a callback.

The example AsyncActionBean cannot be written without a callback either : the http client works with callbacks, so you cannot return anything from the event method. You have to call complete(), that's how non blocking stuff works.

Servlet3 provides the AsyncContext object for that. The AsyncResponse arg is just the counterpart for Stripes.

vankeisb commented 8 years ago

closed by mistake !

rgrashel commented 8 years ago

Ok, after looking at the code and writing a prototype, I can see the issue now. There are actually two use-cases here:

Use-Case 1) Dispatch a request asynchronously which has an event-handler that uses non-asynchronous code inside of it.

Use-Case 2) Dispatch a request asynchronously which has an event-handler that uses asynchronous-code inside of it.

With Use-Case 1, the dispatcher knows when to complete the AsyncContext because the event-handler returns a Resolution after the work is completed. In Use-Case 2, the dispatcher does NOT know when to complete the AsyncContext because the Resolution itself does not know when the asynchronous code inside of it has completed. This is the classic situation where some people kick of threads, and then yet more threads within those threads. So the design approach is that worker threads need to notify the monitor threads when they are complete and then the monitor threads need to notify the application so that it can return the results. Not my personal preference with how to design a system, but I understand that people can and may do this.

The issue I see here is that if people only have Use-Case 1... they are being forced to write code as if they have Use-Case 2. In the example AsyncActionBean, I'd simply have made a blocking HttpClient call because the Resolution itself is already running asynchronously in a different thread pool and therefore is not blocking any request threads.

For Use-Case 2, if you decide to write your already asynchronous method with asynchronous code inside of it, then you have to tell whatever started you that you are complete. But Use-Case 1 should not have to do this, IMHO.

So as the current enhancement stands, I see it satisfying developers who write asynchronous code according to Use-Case 2. But I see it placing an unnecessary burden on developers who have Use-Case 1 and force them to write code in a way that they don't need to. It makes their code unnecessarily ugly, complex, and difficult to debug. If we want a complete and elegant solution, I think we should satisfy Use-Case 1... which is that the dispatching is done asynchronously if an event handler is defined as being asynchronous... and the event handler method can be written exactly as a normal event handler would be written within Stripes.

vankeisb commented 8 years ago

I agree, there is 2 categories of "async servlet usage". Use case 1/ is about submitting tasks to another thread pool, and 2/ is the "real" non blocking, callback based approach.

Both are valid, and used in different situations.

But as you said, the proposed API solves the 2 use cases already :

Use case 1/ :

public void sumitLongTask(AsyncResponse async) {
  executorService.submit(new Runnable() {
    void run() {
      // do anything in the executorService
      ...
      // and finally return a "resolution"
      async.complete(new ForwardResolution('/done.jsp'))
    }
  })
}

Use case 2/ :

public void doSomethingNonBlocking(AsyncResponse async) {
  nonBlockingClient.call(..., new ClientCallback() {
    void execute(MyResult r) {
      // non blocking call responded : do something with the result 
      ...
      // and then "return" a resolution
      async.complete(new ForwardResolution('/done.jsp'))
    }
  });
}

So, if I understand, you'd like to "hide" the AsyncResponse for use case 1/.

Something like this is definitely doable (that's how they do it with Spring btw) :

@Asynchronous
public Resolution doAsync() {
   // usual biz but done in an async fashion
   return new ForwardResolution('/done.jsp')
}

Or

public Callable<Resolution> doAsync() {
    return new Callable<Resolution>() {
        public Resolution call() {
            // biz as usual
            return new ForwardResolution('/done.jsp')
        }
    }
}

(I think the one with an annot is way more Stripey btw)

But, to be honest I'm not sure it is unnecessary.

Again, the two use cases are covered easily with the current code, so I nee no reason to have several ways to do it. I'd rather think it's better to have a single path and not several ones when it comes to using a framework in general.

Also, I think async actions require to understand what asynchronism really means in this context. The void myResolution(AsyncResponse) signature clearly tells you that you're implementing an async action, it's non ambiguous.

On the other hand, I agree that it's unnecessary clutter to call complete() yourself in use case 1.

I'm logging in on IRC right now :P