hk2166 commented on PR #16786:
URL: https://github.com/apache/grails-core/pull/16786#issuecomment-6523021577
Reworked in 4694264b0c and 6ed3af27ec, following @codeconsole's design. The
copy is gone, along with `CompletedHttpServletRequest`, `DispatchView` and
`TaskRequestFilter`.
- **What a task is given:** a `TaskRequest` and a `TaskResponse`
(`HttpServletRequestWrapper`/`HttpServletResponseWrapper`). They call the live
request and response while it is live. `getRequest()`/`getResponse()` return
the live objects, so `getNativeRequest` works.
- **When the container is done:** a `ServletRequestListener` the plugin
registers tells the lifecycle, after any forward, include or asynchronous
processing. From then on, every call to the task's request, response, or a body
stream, reader or writer it took, throws `IllegalStateException("The request
has completed, so a task that outlives it can no longer use its request")`. The
wrappers release the request and response.
- **Calls in flight:** a call in flight finishes first, under a read/write
lock, including a blocking read or write. The calls Tomcat may answer on a lock
the completing thread holds are checked before and after instead:
- asking whether the body is ready;
- setting a read or write listener;
- starting async;
- once a read listener is set, reading the body or parts.
Completion waits at most 60 seconds
(`grails.async.requestCompletionWait`), then detaches with a warning.
- **Nothing is read** from the request as it completes.
- **The task's web request is detached:** `GrailsWebRequest` gets a
protected `isDetached()` hook that `lookup()` honours. Binding then resolves
the format as outside a request (`MimeType.ALL`), as in 7.x.
- **Copies:** parameters, headers and the like are returned as copies,
because Tomcat reuses those objects for the next request. A held reference
therefore never shows another user's request, which was @jdaugherty's concern.
Attribute values are returned as they are.
- **Async completed before the dispatch ends:** the lifecycle listens for
`onComplete` from the first task started once async began. Otherwise it
detaches the tasks once the container recycles the request. Jetty fires
`requestDestroyed` after every dispatch, and Tomcat in `MUST_COMPLETE` reports
async as started when it is done, so the listener call alone can't tell.
@jdaugherty, this keeps the lifecycle centralized, as you wanted from the
copy. Any reference a task captured, including streams, writers and the
response, fails closed in one place, without the copy trying to imitate the
container.
Things I'd point out:
- **Tomcat's async error page:** Tomcat fires `requestDestroyed` for an
async request before it renders the error page. The task used to overwrite the
request's `org.grails.WEB_REQUEST` attribute with its own web request, and the
existing `TimeoutControllerSpec` (a `WebPromises` timeout returning 503) caught
the error page using it after the detach. The task's web request is now bound
to the task's thread only, and the task's request does not show that attribute.
- **Not covered, and in the guide:**
- Code that asks Spring's `RequestContextHolder` directly still finds the
task's web request, whose request throws.
- Objects obtained by unwrapping (`getRequest()`, `getNativeRequest`,
`getPart`, `getSession`) are the request's own and are not guarded.
Before each push I had the change reviewed independently, against Tomcat
11.0.27's and Jetty 12's bytecode, for what you would find next. Fixed from
those reviews:
- An include removing the lifecycle attribute added during it.
- A task started after `requestDestroyed` keeping the request.
- The guard copying attribute values.
- Shared wrappers letting one task's forward change another's.
- Writes and blocking reads not being waited for.
- `startAsync` deadlocking with Tomcat's `AsyncStateMachine`.
- An unbounded wait.
@jamesfredley, your three points from today are fixed in 6ed3af27ec, and the
two from yesterday were in code this replaces. Details inline.
On 4694264b0c, 8 of the new test cases fail, each on its own point: the
three cases of your comments today, `startAsync` and the bounded wait. Of the
tests that use only public API, 7 of 8 fail on `8.0.x` and all 8 on 3847d6f006
(the copy). With 6ed3af27ec, the module's 75 tests pass, and so do
`GrailsWebRequestSpec` and the pubsub demo's 20 integration tests through
Tomcat, including the existing `TimeoutControllerSpec`. Checkstyle, CodeNarc,
PMD and the repository conventions are clean, and the guide builds with the
rewritten paragraph and the new `grails.async.requestCompletionWait` entry in
the configuration reference.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
