Daniel Kulp wrote:
FYI for everyone else: log of a quick IRC chat I had with Willem for part of
my review:
[09:59] <dkulp> Could we avoid using a Message property? The call to
getContextual property over and over again is a bit expensive.
[09:59] <willem> cool, I'm writing a mail for it.
[09:59] <dkulp> I was actually thinking just having a suspend() method on
PhaseInterceptorChain that the continuation would call.
[09:59] <dkulp> sets a boolean right on the chain.
[10:00] <willem> en, that could be more easy to do.
[10:00] <willem> but I need to get the PhaseInterceptorChain instance from the
message.
[10:00] <dkulp> Yep. The message holds it.
[10:01] <dkulp> message.getInterceptorChain()
I'm afraid I had to introduce other status into interceptor chain like
SUSPEND, it will move the interceptor iterator to the preious one and
through the SuspendedInvocationException, and PAUSED will not do these
things.
I will give it a shoot :)
Also, I have one more comment. In the patch, you have:
+ public void onTimeout(AsyncEvent event) throws IOException {
redispatch();
+ isPending = false;
}
....
public boolean suspend(long timeout) {
+ if (isPending) {
+ return false;
+ }
which would completely prevent suspend from being able to be called again
during a redispatch that occurred for a timeout event.
I think the isPending=false needs to move to before the redispatch() call.
Not 100% positive though.
On Wednesday 08 September 2010 10:02:50 am Willem Jiang wrote:
Hi Dan,
Daniel Kulp wrote:
On Thursday 02 September 2010 9:32:46 am Willem Jiang wrote:
Hi
I just did some work to let the camel-cxf leverage the CXF continuation
framework which you did. I just found current CXF continuation which
suspend is implemented by throw the runtime exception. This
implementation has a shortcoming which cannot call the other framework's
async API after continuation suspend is called as Jetty7 does.
This will introduce a situation that continuation resume will be called
before the continuation suspend is called if the famework's async API is
finally using the thread to do the job.
So I'm thinking of some enhancement could be possible to let CXF
continuation leverage Jetty 7 new continuation API.
Any idea?
I created a JIRA[1] and submit a patch for it. Please review it :)
While upgrading things to Jetty 7, this is something I was thinking about
a bit more as well. I actually wanted to add a method like:
public void resume(Object r);
method onto the continuation. The endpoint (or background thread or
whatever) could call that directly (the Object being the thing that would
normally be returned from the method). The runtime could immediately
just grab the associated chain and pick up where it left off and not do
the restart thing. The new Servlet 3 API's and the new things in Jetty
7 seem to allow that. It could potentially make the continuations
stuff a lot easier to work with.
Current we just make the interceptor chain resume from where we pause
it. I don't know that you want.
I think the blow code is enough.
continuation.setObject(obj);
continuation.resume()
I think my point was that why have it resume where it left off at all. Why
could we not just do something like:
Foo doSomething(...) {
....
if (continuationProvider != null) {
final Coninuation cont = coninuationProvider.getContinuation();
cont.suspend();
executor.execute(new Runnable() {
public void run() {
cont.resume(doTheRealThing(....));
}
});
return null;
}
// no continuation support, must do syncronous
return doTheRealThing(...);
}
Foo doTheRealThing(...) {
return new Foo();
}
Understood, in this way the implemenation can support the continuation
and no continuation environment at the same time.
As we get the control of the interceptor chain, it can be done by
hacking the ServiceInvokerInterceptor and put the response back, but it
will break the old continuation semantic which can make the user method
be called again when the continuation resume is called.
Willem
With a setup like that, there isn't a need for doSomething(...) to be called
again at all. The runtime would be able to handle it all. For some
transports, we could use the thread that called resume(obj) to restart the
chain and such and not bother with another transport provided thread and do
the tread context switches and such.
Dan
I actually would like to change everything to not bother with the
exception either. The endpoint would call "suspend" and if it returns
true (meaning the request was suspended), just return null. If false,
it knows it needs to do the work synchronously. It's much closer to
how the Servlet 3 things work. Unfortunately, that would completely
change the semantics of the API and would require some good docs for the
migration guide.
I don't think this is a good idea, if we can't support to the
continuation API from the transport level, we simple don't let the user
can get the ContinuationProvider from the message context.
It could be much easier for user to use :)
[1]https://issues.apache.org/jira/browse/CXF-2982
Willem
