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

PAYARA-2308 Replaced all usages of StringBuffer with StringBuilder where possible #3644

Merged
merged 7 commits into from Feb 5, 2019

Conversation

Projects
None yet
4 participants
@svendiedrichsen
Copy link
Contributor

svendiedrichsen commented Jan 21, 2019

I want to apologize in advance for the size of this PR. I have replaced all usages of StringBuffer with StringBuilder throughout the codebase using my IDEs tools.

I have run the quicklook tests successfully locally.

I can understand if this exceeds the amount of changes you are willing to accept in a single PR. Please feel free to reject.

@Pandrex247

This comment has been minimized.

Copy link
Member

Pandrex247 commented Jan 22, 2019

Yeah that's quite big lol that'll be hard to properly review, particularly since your IDE also seems to have done some import organisation on top of the headline changes. It would be nicer if it was split into multiple PRs based on the modules being changed, though I understand that's quite the faff :P
Having said that, there isn't actually much "meat" to review - a quick scan indicates that the PR is exactly what's written on the tin and this is just pure refactor work, so we'll leave it open for now.

The immediate worry is the obvious one in that StringBuffer is thread-safe, whereas StringBuilder is not, so it's not quite a simple drop and replace since the nature of thread-safety issues can make finding any bugs that these changes would most likely create harder to find.

@svendiedrichsen

This comment has been minimized.

Copy link
Contributor Author

svendiedrichsen commented Jan 22, 2019

... StringBuffer is thread-safe, whereas StringBuilder is not, ...

Sure. But thread-safety generally only comes into play when StringBuffer/StringBuilder is a member of a class whose instances are shared by multiple threads. For this PR there are almost no member replacements but only replacements for local variables within methods. For those few replaced instance variables it looks as if the owning class' instances are not shared between threads. Which of course I can be wrong about but there are only a few cases. I will investigate those.

svendiedrichsen added some commits Jan 23, 2019

Merge branch 'master' of https://github.com/payara/Payara into replac…
…e-all-stringbuffer-with-stringbuilder

# Conflicts:
#	appserver/orb/orb-iiop/src/main/java/org/glassfish/enterprise/iiop/impl/IIOPSSLSocketFactory.java
@svendiedrichsen

This comment has been minimized.

Copy link
Contributor Author

svendiedrichsen commented Jan 23, 2019

@Pandrex247 I have reverted back to StringBuffer the instances which may be accessed multi threaded. Additionally I have merged the master into this branch solving 1 conflict.

Show resolved Hide resolved ...mmon/common-util/src/main/java/com/sun/enterprise/util/io/FileUtils.java Outdated
Show resolved Hide resolved ...main/java/com/sun/jdo/spi/persistence/support/ejb/cmp/CMPBeanHelper.java
String key, String beanName, EjbBundleDescriptor bundle,
Exception e, StringBuffer buf) {
String key, String beanName, EjbBundleDescriptor bundle,
Exception e, StringBuilder buf) {

This comment has been minimized.

@Cousjava

Cousjava Jan 28, 2019

Member

Not sure about thread safety on this function

This comment has been minimized.

@svendiedrichsen

svendiedrichsen Jan 28, 2019

Author Contributor

Doesn't look like it is vulnerable. JDOCodeGenerator.generate method creates new StringBuilder instances upon each call and passes them down to this method.

Show resolved Hide resolved .../org/glassfish/persistence/ejb/entitybean/container/EntityContainer.java
Show resolved Hide resolved .../java/org/glassfish/web/deployment/descriptor/MimeMappingDescriptor.java
Show resolved Hide resolved ...java/org/glassfish/web/deployment/descriptor/UserDataConstraintImpl.java
@svendiedrichsen

This comment has been minimized.

Copy link
Contributor Author

svendiedrichsen commented Jan 28, 2019

@Cousjava I have fixed one of your reviews and answered the other ones. Please have a look.

Merge branch 'master' of https://github.com/payara/Payara into replac…
…e-all-stringbuffer-with-stringbuilder and resolve one conflict.

# Conflicts:
#	appserver/jms/jms-core/src/main/java/com/sun/enterprise/connectors/jms/system/ActiveJmsResourceAdapter.java
@Cousjava

This comment has been minimized.

Copy link
Member

Cousjava commented Feb 4, 2019

Jenkins test please

@MeroRai

MeroRai approved these changes Feb 4, 2019

@Pandrex247 Pandrex247 merged commit 5400417 into payara:master Feb 5, 2019

1 check passed

Payara Quick Build and Test Quick build and test passed!
Details

@svendiedrichsen svendiedrichsen deleted the svendiedrichsen:replace-all-stringbuffer-with-stringbuilder branch Feb 5, 2019

@Cousjava

This comment has been minimized.

Copy link
Member

Cousjava commented Mar 1, 2019

Internal issue PAYARA-2308

@Cousjava Cousjava changed the title Replaced all usages of StringBuffer with StringBuilder where possible PAYARA-2308 Replaced all usages of StringBuffer with StringBuilder where possible Mar 1, 2019

@Pandrex247 Pandrex247 added this to the 5.191 milestone Mar 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.