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

Fix #412 AsyncContext execute #414

Closed
wants to merge 4 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 49 additions & 2 deletions api/src/main/java/jakarta/servlet/AsyncContext.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@

package jakarta.servlet;

import java.util.concurrent.Executor;

/**
* Class representing the execution context for an asynchronous operation that was initiated on a ServletRequest.
*
Expand All @@ -36,9 +38,17 @@
* {@link #dispatch} methods, call {@link #complete}.</li>
* </ol>
*
* <p>
* An AsyncContext is an {@link Executor} that provides mutual exclusion between container managed threads for the same
* request. Specifically container managed threads are mutually excluded from simultaneous calls to
* {@link Servlet#service(ServletRequest, ServletResponse)},
* {@link Filter#doFilter(ServletRequest, ServletResponse, FilterChain)}, {@link ReadListener#onDataAvailable()},
* {@link WriteListener#onWritePossible()} and to execution of the {@link Runnable}s passed to
* {@link AsyncContext#execute(Runnable)}.
*
* @since Servlet 3.0
*/
public interface AsyncContext {
public interface AsyncContext extends Executor {

/**
* The name of the request attribute under which the original request URI is made available to the target of a
Expand Down Expand Up @@ -188,6 +198,11 @@ public interface AsyncContext {
* within the same asynchronous cycle will result in an IllegalStateException. If startAsync is subsequently called on
* the dispatched request, then any of the dispatch or {@link #complete} methods may be called.
*
* <p>
* Best practise is to call this method in a way that is mutually excluded from other threads active on the same
* request. This can be achieved by only calling the method from a container managed thread. A container may issue a
* warning if a non managed thread is used and future versions of the specification may prohibit such calls.
*
* @throws IllegalStateException if one of the dispatch methods has been called and the startAsync method has not been
* called during the resulting dispatch, or if {@link #complete} was called
*
Expand Down Expand Up @@ -218,6 +233,11 @@ public interface AsyncContext {
* <p>
* See {@link #dispatch()} for additional details, including error handling.
*
* <p>
* Best practise is to call this method in a way that is mutually excluded from other threads active on the same
* request. This can be achieved by only calling the method from a container managed thread. A container may issue a
* warning if a non managed thread is used and future versions of the specification may prohibit such calls.
*
* @param path the path of the dispatch target, scoped to the ServletContext from which this AsyncContext was
* initialized
*
Expand Down Expand Up @@ -252,6 +272,11 @@ public interface AsyncContext {
* <p>
* See {@link #dispatch()} for additional details, including error handling.
*
* <p>
* Best practise is to call this method in a way that is mutually excluded from other threads active on the same
* request. This can be achieved by only calling the method from a container managed thread. A container may issue a
* warning if a non managed thread is used and future versions of the specification may prohibit such calls.
*
* @param context the ServletContext of the dispatch target
* @param path the path of the dispatch target, scoped to the given ServletContext
*
Expand All @@ -277,17 +302,40 @@ public interface AsyncContext {
* <tt>startAsync</tt> has returned to the container, then the call will not take effect (and any invocations of
* {@link AsyncListener#onComplete(AsyncEvent)} will be delayed) until after the container-initiated dispatch has
* returned to the container.
*
* <p>
* Best practise is to call this method in a way that is mutually excluded from other threads active on the same
* request. This can be achieved by only calling the method from a container managed thread. A container may issue a
* warning if a non managed thread is used and future versions of the specification may prohibit such calls.
*/
public void complete();

/**
* Causes the container to dispatch a thread, possibly from a managed thread pool, to run the specified
* <tt>Runnable</tt>. The container may propagate appropriate contextual information to the <tt>Runnable</tt>.
*
* @see #execute(Runnable)
* @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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* <p>
* If {@link AsyncContext#complete()} is called, or the request lifecycle completes via a dispatch prior to the runnable
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* being run, then it will never be run.
* <p>
* The container may propagate appropriate contextual information to the <tt>Runnable</tt>.
*
* @see #start(Runnable)
* @param run the asynchronous handler
*/
@Override
public void execute(Runnable run);

/**
* Registers the given {@link AsyncListener} with the most recent asynchronous cycle that was started by a call to one
* of the {@link ServletRequest#startAsync} methods.
Expand Down Expand Up @@ -405,5 +453,4 @@ public interface AsyncContext {
* @return the timeout in milliseconds
*/
public long getTimeout();

}