-
Notifications
You must be signed in to change notification settings - Fork 126
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
Explore safey of YamlWireOut#sb re-use #877
Conversation
@Before | ||
public void ignorePoolCapacityExceededErrors() { | ||
// Remove this to see where we have problematic re-use of YamlWire#acquireStringBuffer | ||
ignoreException("Pool capacity exceeded, consider increasing maxInstances"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Get rid of this ignore to see where we might have an issue
@@ -85,7 +88,7 @@ public abstract class YamlWireOut<T extends YamlWireOut<T>> extends AbstractWire | |||
} | |||
|
|||
protected final YamlValueOut valueOut = createValueOut(); | |||
protected final StringBuilder sb = new StringBuilder(); | |||
protected final ScopedResourcePool<StringBuilder> sb = StringBuilderPool.createThreadLocal(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this probably doesn't need to be a thread-local given that YamlWireOut is single-threaded anyway. Making a single-threaded implementation of ScopedResourcePool is trivial.
42dd0ff
to
b03500f
Compare
b03500f
to
7fdaf26
Compare
|
I ignored the warning that indicates potentially unsafe re-use but there's a lot of them
They're not necessarily unsafe, it just indicates where sb is use in an outer scope then again in an inner scope. It could be that the outer scope doesn't care that the buffer gets trashed. Or it could be an issue like we've seen.
There are probably lots of "problematic" re-use patterns that look like this:
Which is actually not problematic, it's a false-positive to say that the above represents unsafe re-use. The way to solve that is to tighten up the outer scope e.g.