Created attachment 39323 [details] Java Flight Recording with spring boot application Since fixing bug 66196, the org.apache.tomcat.buf.MessageBytes.toString() is no longuer cached. For one tomcat request, if a multiple calls (by example to this method, org.apache.catalina.connector.RequestFacade.getMethod() by springframework) is processed, a memory overconsumption is observed Maybe just fix with this ? ``` public String toString() { switch (type) { case T_NULL: case T_STR: // No conversion required break; case T_BYTES: // strValue = byteC.toString(); #OLD setString(byteC.toString()); break; case T_CHARS: // strValue = charC.toString(); #OLD setString(charC.toString()); break; } return strValue; } ``` Same problem with tomcat - >= 10.1.0 - >= 9.0.71 - >= 8.5.85
Actually, the change that actually modified the behavior is the fix for bug 66627 ( https://github.com/apache/tomcat/commit/897931b68c89788eeb71398c8e6dfda1d5bae161 ). Obviously the entire thing is not going to be reverted. Basically it is now up to the caller to decide to call MessageBytes.setString manually as needed, to avoid repeated conversions that could cause some GC. Since this is annoying boilerplate code, maybe a new utility toString method could be introduced (toStringType ?) and update callers as needed.
Maybe, it's possible to cache in the caller ? https://github.com/apache/tomcat/blob/main/java/org/apache/catalina/connector/Request.java For all coyoteRequest.*.toString() calls ?
Or maybe just update with this code to keep the behaviour described in the getType() Javadoc ? public void setBytes(byte[] b, int off, int len) { byteC.setBytes(b, off, len); type = T_BYTES; hasHashCode = false; hasLongValue = false; strValue = byteC.toString(); } /** * Sets the content to be a char[] * * @param c the chars * @param off the start offset of the chars * @param len the length of the chars */ public void setChars(char[] c, int off, int len) { charC.setChars(c, off, len); type = T_CHARS; hasHashCode = false; hasLongValue = false; strValue = charC.toString(); } // -------------------- Conversion and getters -------------------- /** * Compute the string value. * * @return the string */ @Override public String toString() { return strValue; }
Changes to the behaviour of MessageBytes has previously introduced subtle bugs. I am therefore extremely reluctant to make the changes suggested in comment #0 or comment #3. Caching the values as suggested in comment #2 creates the risk that changes made to the underlying MessageBytes instance will not be seen. I don't think that is the case with request method but I'd prefer a more general solution. Rémy's suggestion in comment #1 should work. I'll implement that for request method. I'll look at the other MessageByte usages in coyote request and may update some of the other usages as well.
Fixed in: - 11.0.x for 11.0.0-M14 onwards - 10.1.x for 10.1.16 onwards - 9.0.x for 9.0.83 onwards - 8.5.x for 8.5.96 onwards
Thanks for your quick fix. But if you call toStringType() and after toString() method, your toString() method has not the same result because type attribute has been updated. Maybe it's necessary to take a specific cache in toStringType() without call setString() method ? On the other hand, this should fix the memory consumption problem.