[TCK Challenge] object identity test on ServletRequests in listeners vs servlets

[janbartel]

Challenged Tests:

com.sun.ts.tests.servlet.api.jakarta_servlet.srattributeevent#addedTest
com.sun.ts.tests.servlet.api.jakarta_servlet.srattributeevent#removedTest
com.sun.ts.tests.servlet.api.jakarta_servlet.srattributeevent#replacedTest

TCK Version: Jakarta EE Platform TCK 10.0.0

Tested Implementation: Jetty 12

Description: The above attribute listener tests all use object identity to test that the ServletRequest instance provided to the SRAttributeListener equals the ServletRequest provided to the TestServlet like this:

https://github.com/jakartaee/platform-tck/blame/master/src/com/sun/ts/tests/servlet/api/jakarta_servlet/srattributeevent/TestServlet.java#L114

There is no requirement in the specification to support that use of request object identity.

[joakime]

Wasn't Object Identity entirely dropped from the latest Servlet spec?

[gregw]

Unfortunately I think this is an error in the specification. We removed object wrapper identity from the RequestDispatcher mechanism, but not from filter chains. Specifically 6.0 6.2.2 says:

In order to support this style of filter the container must support the following requirements:

• When a filter invokes the doFilter method on the container’s filter chain implementation, the container must ensure that the request and response objects that it passes to the next entity in the filter chain, or to the target web resource if the filter was the last in the chain, is the same object that was passed into the doFilter method by the calling filter.
• When a filter or servlet calls RequestDispatcher.forward or RequestDispatcher.include, then the request and response objects seen by the called filter(s) and/or servlet must either: be the same wrapper objects that were passed; or wrappers of the objects that were passed.
• When startAsync(ServletRequest, ServletResponse) is used to commence an asynchronous cycle then the request and response objects seen by any filter(s) and/or servlet subsequent to an AsyncContext.dispatch() (or overloaded variant) must either: be the same wrapper objects that were passed; or wrappers of the objects that were passed.

So we can see in the dispatch and async dispatch cases we have changed to the language: "the same wrapper objects that were passed; or wrappers of the objects that were passed", but for the filter chain we still just say: "is the same object that was passed".

So I think the TCK is technically correct... but I'm pretty sure the intention was to allows wrappers.

Perhaps the test should be changed anyway to have a check that allows wrappers?

[gregw]

Note that the reason Jetty wraps in the filter chain is that we now have an immutable request object, so to change the async supported state we wrap the request rather than mutate it.

[markt-asf]

The proposal and change discussion that I can find in the archives was only to remove the object wrapper identify requirement for dispatches: https://github.com/jakartaee/servlet/issues/411#issuecomment-888784325 https://github.com/jakartaee/servlet/pull/413#issuecomment-934876221

Filters were not part of that discussion. The current specification text and TCK looks to be correct to me.

[gregw]

@markt-asf yeah the discussion was only for dispatch... but mentally I think of filter to filter to servlet chaining as dispatches, so it was an oversight on my behalf not to discuss it. It is still a very bad practice to use this mechanism between filters and servlet:

• requires downcasting
• can only be used once, so if two filters wrap, only the later will arrive at the servlet in a way that can be down cast.
• there are still async races that may apply as the request passed now must be mutated with changes such as async supported and runAs. Thus a thread that is passed the request will have a moment that it sees the wrong values for those as the caller exits the filter chain.

I don't think this is a huge problem... but it is a nasty hang over from a very bad idea that we almost but not quite fully removed from 6.0.

I think it would be good to fix in a future spec, but for now it is not a TCK issue.

[markt-asf]

OK, closing this challenge issue then.