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

support for async action beans #37

Open
vankeisb opened this issue Nov 17, 2015 · 40 comments
Open

support for async action beans #37

vankeisb opened this issue Nov 17, 2015 · 40 comments
Assignees

Comments

@vankeisb
Copy link
Member

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
Copy link

iluvtr commented Dec 14, 2015

That would be really nice!

@kfowlks
Copy link

kfowlks commented Dec 20, 2015

+1 On this feature

@timothystone
Copy link

+1

@vankeisb
Copy link
Member Author

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
Copy link

iluvtr commented Dec 24, 2015

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
#37 (comment)
.

@vankeisb
Copy link
Member Author

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 vankeisb self-assigned this Dec 24, 2015
@vankeisb
Copy link
Member Author

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.

vankeisb added a commit that referenced this issue Dec 28, 2015
@iluvtr
Copy link

iluvtr commented Dec 31, 2015

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
#37 (comment)
.

@vankeisb
Copy link
Member Author

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
#37 (comment)
.


Reply to this email directly or view it on GitHub.

vankeisb added a commit that referenced this issue Jan 13, 2016
vankeisb added a commit that referenced this issue Jan 13, 2016
vankeisb added a commit that referenced this issue Jan 13, 2016
vankeisb added a commit that referenced this issue Jan 18, 2016
…cutorService needed. Async resolutions are handled as if blocking from trip.execute(). Behavior should be transparent for the test (sync or async, it should block)
vankeisb added a commit that referenced this issue Jan 18, 2016
vankeisb added a commit that referenced this issue Jan 18, 2016
@vankeisb
Copy link
Member Author

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
Copy link

iluvtr commented Jan 18, 2016

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
#37 (comment)
.

@vankeisb
Copy link
Member Author

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<Resolution>

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
Copy link

iluvtr commented Jan 18, 2016

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
#37 (comment)
.

@vankeisb
Copy link
Member Author

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

@vankeisb
Copy link
Member Author

No need for another @FunctionalInterface here

@iluvtr
Copy link

iluvtr commented Jan 18, 2016

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
#37 (comment)
.

@vankeisb
Copy link
Member Author

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

@rgrashel
Copy link
Member

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
#37 (comment)
.

@vankeisb
Copy link
Member Author

@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...

vankeisb added a commit that referenced this issue Jan 18, 2016
…or reuse of Resolution classes. As the name says, completes the async processing. You cannot use several Resolutions when async processing.
@roncking
Copy link

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
Copy link
Member

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
#37 (comment)
.

@roncking
Copy link

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
<
#37 (comment)

.


Reply to this email directly or view it on GitHub
#37 (comment)
.

@vankeisb
Copy link
Member Author

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
Copy link
Member Author

@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
Copy link
Member Author

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
Copy link

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
#37 (comment)
.

@rgrashel
Copy link
Member

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
<
#37 (comment)

.


Reply to this email directly or view it on GitHub
#37 (comment)
.

@roncking
Copy link

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
Copy link
Member

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
#37 (comment)
.

@vankeisb
Copy link
Member Author

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
Copy link

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
#37 (comment)
.

@vankeisb
Copy link
Member Author

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 :

  • it returns void
  • it accepts a single arg of type AsyncResolution

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 added a commit that referenced this issue Jan 23, 2016
…AsyncResolution). AsyncResolution is no more a base class to extend but instead it's a param to the event handler that allows to complete() the async processing.
vankeisb added a commit that referenced this issue Jan 27, 2016
vankeisb added a commit that referenced this issue Jan 27, 2016
vankeisb added a commit that referenced this issue Jan 27, 2016
@vankeisb
Copy link
Member Author

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.

vankeisb added a commit that referenced this issue Jan 27, 2016
@iluvtr
Copy link

iluvtr commented Jan 27, 2016

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
#37 (comment)
.

@vankeisb
Copy link
Member Author

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

@rgrashel
Copy link
Member

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
Copy link
Member Author

@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
Copy link
Member Author

closed by mistake !

@vankeisb vankeisb reopened this Jan 27, 2016
@rgrashel
Copy link
Member

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
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants