Fix #412 AsyncContext execute #414
Conversation
Fix jakartaee#412 AsyncContext is an Executor that mutually excludes from other container invocations for the same request.
reformat
|
@markt-asf @stuartwdouglas your thoughts on this one? I think it is a good addition to improve the asynchronous usage of the servlet container. Currently we mutually exclude all the container callbacks, but then let an arbitrary thread call |
|
Conceptually I like it. The implementation is not super simple if you have IO threads in the mix though. For performance reasons you will likely want to execute I am also not really sure about |
|
I support the general idea. I'm a little concerned about adding this so late in the day but the fact that the impact for 6.0 will be warnings at most should mitigate any compatibility risks. At a detail level I think we should avoid the use of the word "dispatch" in the Javadoc for the new methods in case it cases confusion with the existing dispatch methods. |
|
@stuartwdouglas The idea is that in future, such tasks would be the preferred (only?) way of doing a I'll ponder stand by.... |
|
@markt-asf @stuartwdouglas I've updated the text a bit and removed |
| * @param run the asynchronous handler | ||
| */ | ||
| public void start(Runnable run); | ||
|
|
||
| /** | ||
| * Execute the passed {@link Runnable} mutually excluded from all other container managed threads for the same request. | ||
| * The execution may be done by the calling thread, which will momentarily be a container managed thread; or by another |
There was a problem hiding this comment.
IMHO this should either always dispatch or always run in the same thread (with my preference being always to dispatch). I can't see this doing anything but causing problems (e.g. if the calling thread holds a lock sometimes the runnable will be run with the lock held, sometimes not).
There was a problem hiding this comment.
Maybe I'm over optimizing. I think that async applications might want to avoid the extra thread dispatch. So if we always use another thread, apps wont use this. If we don't use another thread then we may block in this method.
Give me another day to ponder.
| * Execute the passed {@link Runnable} mutually excluded from all other container managed threads for the same request. | ||
| * The execution may be done by the calling thread, which will momentarily be a container managed thread; or by another | ||
| * container managed thread, possibly from a managed thread pool. Since execution of the {@link Runnable} will exclude | ||
| * other container managed methods, this method is not intended for long-running tasks, nor ones that may block. |
There was a problem hiding this comment.
So is the expectation here that this will be run on the IO thread rather than an executor? If it is run on a normal executor then IMHO long running or blocking tasks are likely fine, it just needs a warning that if you are doing async IO it will basically be suspended.
There was a problem hiding this comment.
well it is the same issue as existing issue with onWritePossible, onDataAvailable and/or service mutually excluding each other, so any long running tasks will prevent the other callbacks. I guess we don't currently warn against long-running tasks. I'll reword.
| * container managed thread, possibly from a managed thread pool. Since execution of the {@link Runnable} will exclude | ||
| * other container managed methods, this method is not intended for long-running tasks, nor ones that may block. | ||
| * <p> | ||
| * If {@link AsyncContext#complete()} is called, or the request lifecycle completes via a dispatch prior to the runnable |
There was a problem hiding this comment.
I really don't like this either, it means that you can never trust the Runnable to clean up resources, if you have DB connections that need to be returned to a pool or things of that nature you also need to register a listener.
There was a problem hiding this comment.
Hmmmm the issue this was trying to fix was to stop an async thread accessing a request/response object that had been recycled and was being used on another request cycle. However, with the removal of Object Wrapper Identity, this may not be needed any more as referenced to requests can be made durable if a container wishes.
I'll ponder and probably remove.
|
I'm going to close this unmerged for now. This is something that can easily be added in a 6.1 once we get a bit more experience about how the current changes in 6.0 will affect async handling... specifically durable request will make thing much better for async handling. |
Fix #412 AsyncContext is an Executor that mutually excludes from other container invocations for the same request.