Bug 56399

The feature (but not the refactoring it implied) was implemented in Tomcat 8 with r1594436 and will be in 8.0.6.

No it will not. The commit for this was explicitly excluded from the 8.0.6 tag. Additionally, the commit has been vetoed so it is unlikely that the fix will remain in its current form.

After testing some more, I don't see any problems besides the one TestAbstractHttp11Processor test (as seen on buildbot), but it's still best to only include it in the next version.

The commit could be improved (besides trying to identify the problem with the test):
- no apparent benefit of logging a stacktrace since it does not give any additional info; if this is done for visibility in logs then I disagree with that trick
- checking the request should be enough (although I would like to get info if this is meant to try to detect bad async access too, in which case checking the response commit state is a good idea)

Another related topic: the amount of WebappClassLoader.clearReferencesThreads logging that happens running the testsuite. This feature is a good idea, but with a lot of noise.

(In reply to Remy Maucherat from comment #3)
> The commit could be improved (besides trying to identify the problem with
> the test):
> - no apparent benefit of logging a stacktrace since it does not give any
> additional info; if this is done for visibility in logs then I disagree with
> that trick

The goal here is to protect against unexpected bugs in request handling, as leaking unrecycled request or response has rather bad consequences.
The stack trace is needed for us to identify problem code, as there are a dozen of ways in how processor recycling can be triggered. I expect such bugs to be rare and hard to reproduce, thus a stack trace is helpful.

As the original cause of the problem is not some condition under administrator's control, but rather some problem in Tomcat code, I agree that using "warning" log level here is a nuisance. That is my fault.

I will reduce the logging level to "info".
It can be further reduced by help of UserDataHelper, but that strikes me as premature optimization.

> - checking the request should be enough (although I would like to get info
> if this is meant to try to detect bad async access too, in which case
> checking the response commit state is a good idea)

The request check can be simplified to check getHost() only. It essentially means that request line has been read and request has been run through Mapper.

The response check essentially means that response.outputbuffer has not been recycled. The call returns a sum of two counters.

In general request check should be enough.
If request check was OK but response one fails, then maybe

a) mapping failed (host==null), but some response was written.
b) some broken webapp is still holding a reference to response object and is writing some data.

As b) is more likely, maybe the response check fail should be treated differently, as it is likely a problem in web application code, not in Tomcat.

In reply to Mark's e-mail on "Re: r1594436"
2014-05-14 12:02 GMT+04:00 Mark Thomas:

> Why is an exception being used for flow control? Surely a boolean flag
> would be better here especially as this is being called on every
> request.

Ack.
In some sense those are remnants of older code that I had before simplifying the bits. I do not think those slow down things (as throwing the exceptions is an unexpected use case).

I still think that exceptions are needed for the log statements, but we can generate the exceptions only when the log messages are actually written out.

As I mentioned in comment 4, it would be better with separate message texts for unrecycled request vs. unrecycled response.

(BTW, The mailing list still fails as a place for discussions. This particular message was delayed by 48 hours).

2014-05-14 12:02 GMT+04:00 Mark Thomas:

> I remain of the view that a better solution would be to recycle both
> pairs of request and response objects at the same time.

The idea of "to create a new Recyclable interface in Coyote" (from Comment 0) is not doable. The "notes" in coyote request are opaque. The CoyoteAdapter.ADAPTER_NOTES constant is defined in Catalina only.
(In coyote it is mentioned in javadoc for coyote.Request#setNote()).

Thus a better solutions looks to introduce some method into Adapter interface.
(Or rename and adapt the "checkRecycled()" or "log()" one).

Will the recycling driven by Coyote or by Catalina? Will some code from Catalina be removed or simplified?

Will the need to recycle accompanied by some action?

Will it be possible to backport the changes to earlier versions?

Created attachment 31633 [details]
Do not use exception for flow control (2014-05-16_tc8_CoyoteAdapter.patch)

(In reply to comment #5)

Patch that changes the implementation to do not use an exception for flow control.

I did not manage to commit it before 8.0.8 tag. I will wait awhile (a day or two) before proceeding with it.

Improvements from the above comments were committed to trunk in r1595887 and will be in 8.0.9.

Backported to Tomcat 7 in r1595900 to be in 7.0.54.

(In reply to Remy Maucherat from comment #3)
>
> Another related topic: the amount of
> WebappClassLoader.clearReferencesThreads logging that happens running the
> testsuite. This feature is a good idea, but with a lot of noise.

I filed that as a separate issue:
https://issues.apache.org/bugzilla/show_bug.cgi?id=56546

This has been fixed to the extent it can given the current architecture.