Refactor ByteArrayOutputStream into synchronized and non-synchronized versions by adamretter · Pull Request #108 · apache/commons-io

adamretter · 2020-04-03T10:45:26Z

This abstracts the functionality of ByteArrayOutputStream into AbstractByteArrayOutputStream. Synchronized and non-synchronized (for performance) implementations are then provided in ByteArrayOutputStream and FastByteArrayOutputStream.

Tests are also provided.

p.s. I have just submitted by ICLA by email.

…rsions

coveralls · 2020-04-03T11:03:00Z

Coverage increased (+0.2%) to 89.588% when pulling 568e3e7 on adamretter:iostreams-sync into 335808a on apache:master.

aherbert

This looks like a nice conversion and maintains the previous support. I'd be wary of using a new dependency just to mark the code as Threadsafe with an annotation on an existing class. Bringing in an annotation framework is a different topic.

Using the name FastXXX is presumptuous but I cannot think of something better. NonSyncXXX is not as friendly. Others may have comments on this.

aherbert · 2020-04-03T11:01:16Z

  <dependencies>
+    <dependency>
+      <groupId>com.google.code.findbugs</groupId>
+      <artifactId>jsr305</artifactId>


Is this just to bring in the @javax.annotation.concurrent.ThreadSafe annotation? I don't think this necessary since you already document the thread safety in the javadoc. Nothing else in the code base uses this non-standard annotation support.

Well yes it is to bring in the annotation... but these annotations can also be used by various tools like FindBugs and others to detect issues.

I think it adds value, and jsr305 is tiny (19KB). I can remove it if people prefer? It also adds useful annotations like @Nullable and @NotNull for similar purposes.

I can remove it though if that is preferred. Should I do that?

I would remove it from this PR. Adding an annotation dependency should be in another PR that perhaps then makes more use of it than a single location. The Guava codebase uses these throughout and in that case they have value for a bug detection tool. I don't think a single annotation of @ThreadSafe is likely to detect many (any?) bugs given that synchronized code should be less of a problem than unsynchronized code. If you do have a bug detection tool that flags this class as a problem when used in a concurrent scenario a brief look at the javadoc (which notes this is safe for concurrent use) will allow you to mark the bug as false positive.

Okie dokie. Done.

aherbert · 2020-04-03T11:01:48Z

+
+    private static Stream<Arguments> baosFactories() {
+        final BAOSFactory syncBaosFactory = size -> new ByteArrayOutputStream(size);
+        final BAOSFactory nonSyncBaos = size -> new FastByteArrayOutputStream(size);


rename nonSyncBaosFactory

…asses

garydgregory · 2020-04-03T17:51:21Z

This looks like a nice conversion and maintains the previous support. I'd be wary of using a new dependency just to mark the code as Threadsafe with an annotation on an existing class. Bringing in an annotation framework is a different topic.

Using the name FastXXX is presumptuous but I cannot think of something better. NonSyncXXX is not as friendly. Others may have comments on this.

I am a big -1 on calling a new class "Fast" since it is not fast, it just removes thread-safety. You can hope that it is "faster" than the standard class but that's it.

The JRE has been calling some thread-safe classes Concurrent like ConcurrentHashMap but other JRE thread-safe classes just have names that reflect behavior like CopyOnWriteArrayList, it therefore seem OK to me to call a classes that is not synchronized one of:

UnsynchronizedByteArrayOutputStream (to the point, precise)
SimpleByteArrayOutputStream (not great)
BasicByteArrayOutputStream (not great)
DefaultByteArrayOutputStream (not great)

My preference is UnsynchroniziedByteArrayOutputStream.

adamretter · 2020-04-03T18:00:42Z

@garydgregory Okay I understand. Your preferred option is very long though. How do you feel about:

UnsyncByteArrayOutputStream.
UByteArrayOutputStream.
USByteArrayOutputStream.
UnsafeByteArrayOutputStream.

garydgregory · 2020-04-03T18:13:44Z

@garydgregory Okay I understand. Your preferred option is very long though. How do you feel about:

UnsyncByteArrayOutputStream.

The least worst IMO.

UByteArrayOutputStream.

Oh, the horror! ;-) There is no convention that equates "U" to "Unsynchronized", if anything "U" usually stands for "Unsigned".

USByteArrayOutputStream.

Even worse IMO! Same comment as above except US usually means... United States?

UnsafeByteArrayOutputStream.

Nope, same as for "Fast", unsafe compared in what area? Will this be a CVE?

I do not think there should be an issue spelling out Unsynchronized, if you look around, you'll see similar names spelled out:

javax.transaction.Synchronization
java.util.concurrent.locks.AbstractOwnableSynchronizer
java.util.concurrent.locks.AbstractQueuedLongSynchronizer
java.util.concurrent.locks.AbstractQueuedSynchronizer
java.util.Collections.SynchronizedCollection
java.util.Collections.SynchronizedList
java.util.Collections.SynchronizedMap
java.util.Collections.SynchronizedNavigableMap
java.util.Collections.SynchronizedNavigableSet
java.util.Collections.SynchronizedRandomAccessList
java.util.Collections.SynchronizedSet
java.util.Collections.SynchronizedSortedMap<
java.util.Collections.SynchronizedSortedSet
Too many to list in Google Guava.
Too many to list in our own Apache Commons Collections.

So, IMO, just say what it does.

adamretter · 2020-04-03T19:11:35Z

@garydgregory Fair points. I have renamed it as you suggested.

garydgregory · 2020-04-03T20:44:14Z

+ * like the original. The only exception is the deprecated
+ * {@link java.io.ByteArrayOutputStream#toString(int)} method that has been
+ * ignored.
+ */


Please Add @since 2.7.

garydgregory · 2020-04-03T20:44:54Z

+ * <p>
+ * The data can be retrieved using <code>toByteArray()</code> and
+ * <code>toString()</code>.
+ * <p>


Please close HTML tags.

garydgregory · 2020-04-03T20:45:26Z

+    }
+
+    /**
+     * Write the bytes to byte array.


"Write" -> "Writes".

garydgregory · 2020-04-03T20:45:53Z

+    }
+
+    /**
+     * Return the current size of the byte array.


"Return" -> "Returns".

garydgregory · 2020-04-03T20:47:04Z

+     * using the platform default charset.
+     * @return the contents of the byte array as a String
+     * @see java.io.ByteArrayOutputStream#toString()
+     * @deprecated 2.5 use {@link #toString(String)} instead


I do not think it makes sense to deprecate toString() since it is such a basic JRE method that will never go away.

This was how it was previously in ByteArrayOutputStream. I just left it as I found it!

garydgregory · 2020-04-03T20:47:51Z

+import java.io.OutputStream;
+
+/**
+ * This class implements a version of {@link AbstractByteArrayOutputStream}


"This class implements..." -> "Implements..."

adamretter · 2020-04-05T12:34:10Z

@garydgregory I have incorporated your further feedback now.

adamretter · 2020-04-05T14:10:24Z

@garydgregory Adding the closing  tags as you suggested breaks the javadoc build, so I will go ahead and remove them again:

[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  06:02 min
[INFO] Finished at: 2020-04-05T12:41:14Z
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-javadoc-plugin:3.1.1:javadoc (default-cli) on project commons-io: An error has occurred in Javadoc report generation: 
[ERROR] Exit code: 1 - /home/travis/build/adamretter/commons-io/src/main/java/org/apache/commons/io/output/ByteArrayOutputStream.java:115: error: unexpected end tag: </p>
[ERROR]      * </p>
[ERROR]        ^
[ERROR] /home/travis/build/adamretter/commons-io/src/main/java/org/apache/commons/io/output/ByteArrayOutputStream.java:142: error: unexpected end tag: </p>
[ERROR]      * </p>
[ERROR]        ^
[ERROR] /home/travis/build/adamretter/commons-io/src/main/java/org/apache/commons/io/output/UnsynchronizedByteArrayOutputStream.java:112: error: unexpected end tag: </p>
[ERROR]      * </p>
[ERROR]        ^
[ERROR] /home/travis/build/adamretter/commons-io/src/main/java/org/apache/commons/io/output/UnsynchronizedByteArrayOutputStream.java:138: error: unexpected end tag: </p>
[ERROR]      * </p>
[ERROR]        ^
[ERROR] 
[ERROR] Command line was: /usr/local/lib/jvm/openjdk11/bin/javadoc @options @packages

aherbert · 2020-04-05T21:51:28Z

Looking at the line errors for your  markers they are all at lines following a <ul> element that cannot be nested within a paragraph. Move the  to before the <ul>.

adamretter · 2020-04-06T10:43:38Z

@aherbert Good point. I have incorporated the changes as you suggested.

garydgregory · 2020-04-07T14:52:48Z

Hi @adamretter
Thank you for your updates.
JaCoCo reports that the code coverage for UnsynchronizedByteArrayOutputStream is less than it is for ByteArrayOutputStream.java. May you address this please?
Thank you!
You can run this: mvn -V clean install site -P jacoco -P japicmp

adamretter · 2020-04-07T16:05:50Z

@garydgregory Hmm, from what I read in coveralls.io the code coverage after this PR is +0.07% so it is a net improvement, no?

garydgregory · 2020-04-07T16:11:29Z

@garydgregory Hmm, from what I read in coveralls.io the code coverage after this PR is +0.07% so it is a net improvement, no?

Please run the command I provided and look at the JaCoCo report. You will see that the new class has less coverage than the current (refactored) one. We should strive to give the best possible code coverage for changes and new code.

adamretter · 2020-04-07T16:20:04Z

@garydgregory Sure I see it now. I have just pushed a commit to further improve coverage.

versions #108.

adamretter · 2020-04-08T10:38:36Z

@garydgregory @aherbert Yay! Thanks :-)

unsynchronized versions #108. Simplify method name.

@aherbert

… versions (apache#108) * Split ByteArrayOutputStream into synchronized and non-synchronized versions * Improve the test coverage of AbstractByteArrayOutputStream and sub-classes * Address review comments by @aherbert * Address review comments by @garydgregory * Address further review comments by @garydgregory * Remove tags, breaks the javadoc build * Address review comments by @aherbert * Improve coverage of tests

versions apache#108.

unsynchronized versions apache#108. Simplify method name.

Split ByteArrayOutputStream into synchronized and non-synchronized ve…

451cf80

…rsions

aherbert requested changes Apr 3, 2020

View reviewed changes

adamretter added 2 commits April 3, 2020 13:55

Improve the test coverage of AbstractByteArrayOutputStream and sub-cl…

6630170

…asses

Address review comments by @aherbert

64fbcf7

adamretter force-pushed the iostreams-sync branch from 4db1a3f to 64fbcf7 Compare April 3, 2020 13:42

Address review comments by @garydgregory

c2a92e3

garydgregory requested changes Apr 3, 2020

View reviewed changes

Address further review comments by @garydgregory

bcadb75

Remove tags, breaks the javadoc build

b60165d

Address review comments by @aherbert

70e2741

adamretter mentioned this pull request Apr 6, 2020

Consistent License Declarations eXist-db/exist#3305

Merged

15 tasks

Improve coverage of tests

568e3e7

garydgregory merged commit c2c2709 into apache:master Apr 8, 2020

asfgit pushed a commit that referenced this pull request Apr 8, 2020

Refactor ByteArrayOutputStream into synchronized and unsynchronized

3c96ee9

versions #108.

This was referenced Apr 8, 2020

Unsynchronized ByteArrayInputStream implementation #109

Merged

Add functional interfaces IOFunction and IOSupplier #110

Merged

asfgit pushed a commit that referenced this pull request Apr 9, 2020

[IO-662] Refactor ByteArrayOutputStream into synchronized and

5004792

unsynchronized versions #108. Simplify method name.

brettlounsbury pushed a commit to brettlounsbury/commons-io that referenced this pull request May 28, 2020

Refactor ByteArrayOutputStream into synchronized and unsynchronized

b6344d3

versions apache#108.

brettlounsbury pushed a commit to brettlounsbury/commons-io that referenced this pull request May 28, 2020

[IO-662] Refactor ByteArrayOutputStream into synchronized and

5c32612

unsynchronized versions apache#108. Simplify method name.

Uh oh!

Conversation

adamretter commented Apr 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Apr 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aherbert left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamretter Apr 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garydgregory commented Apr 3, 2020

Uh oh!

adamretter commented Apr 3, 2020

Uh oh!

garydgregory commented Apr 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamretter commented Apr 3, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamretter commented Apr 5, 2020

Uh oh!

adamretter commented Apr 5, 2020

Uh oh!

aherbert commented Apr 5, 2020

Uh oh!

adamretter commented Apr 6, 2020

Uh oh!

garydgregory commented Apr 7, 2020

Uh oh!

adamretter commented Apr 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

garydgregory commented Apr 7, 2020

Uh oh!

adamretter commented Apr 7, 2020

Uh oh!

adamretter commented Apr 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

adamretter commented Apr 3, 2020 •

edited

Loading

coveralls commented Apr 3, 2020 •

edited

Loading

adamretter Apr 3, 2020 •

edited

Loading

garydgregory commented Apr 3, 2020 •

edited

Loading

adamretter commented Apr 7, 2020 •

edited

Loading