Skip to content
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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

nicktindall
Copy link
Contributor

@nicktindall nicktindall commented Apr 18, 2024

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:

void doSomething() {
    //...
   try (ScopedResource<StringBuilder> sbR = acquireStringBuilder()) {
      StringBuilder sb = sbR.get();
      readFieldName(sb);
      if (sb.contentEquals("something")) {
         doSomethingElse():
      }
   }
}

void doSomethingElse() {
   try (ScopedResource<StringBuilder> sbR = acquireStringBuilder()) {
      StringBuilder sb = sbR.get();
      //...
   }
}

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.

void doSomething() {
   //...
   if (fieldNameIs("something")) {
      doSomethingElse():
   }
}

void doSomethingElse() {
   try (ScopedResource<StringBuilder> sbR = acquireStringBuilder()) {
      StringBuilder sb = sbR.get();
      //...
   }
}

boolean fieldNameIs(String fieldName) {
   try (ScopedResource<StringBuilder> sbR = acquireStringBuilder()) {
      StringBuilder sb = sbR.get();
      readFieldName(sb);
      return sb.contentEquals(fieldName);
   }
}

@Before
public void ignorePoolCapacityExceededErrors() {
// Remove this to see where we have problematic re-use of YamlWire#acquireStringBuffer
ignoreException("Pool capacity exceeded, consider increasing maxInstances");
Copy link
Contributor Author

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);
Copy link
Contributor Author

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.

Copy link

sonarcloud bot commented Apr 19, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants