[IO-826] Add runtime exception support to broken streams #530
Conversation
…enOutputStream / BrokenWriter following example of BrokenInputStream.
…OutputStream / BrokenWriter.
|
While this may not break source compatibility, it would break binary compatibility per https://docs.oracle.com/javase/specs/jls/se8/html/jls-13.html#jls-13.4.12 (at least how I read it and how japicmp sees it). |
|
Yes, you're absolutely right. I hadn't thought of that. |
| */ | ||
| @Override | ||
| public synchronized void reset() throws IOException { | ||
| public void reset() throws IOException { |
There was a problem hiding this comment.
Keep synchronized, there is no reason to break the superclass' specification here.
There was a problem hiding this comment.
Could you clarify? I can't see any mention of synchronized in the javadoc.
There was a problem hiding this comment.
Uh? Javadoc doesn't document synchronized keywords. Same for native, see https://docs.oracle.com/en/java/javase/11/javadoc/javadoc-command.html#GUID-B0079316-8AA3-475B-8276-6A4095B5186A
There was a problem hiding this comment.
I didn't explain myself very well. I meant to say I don't see any mention of locking in the javadoc.
I dug a bit deeper and it seems the synchronization of the reset method varies by JDK. Here's a summary of which JDKs mark it as synchronized, sampled from the ones I had to hand:
| java.io.InputStream#reset | java.io.Reader#reset | |
|---|---|---|
| openjdk-7 | ✅ | ❌ |
| corretto-1.8.0_392 | ✅ | ❌ |
| temurin-11.0.21 | ✅ | ❌ |
| corretto-20.0.2.10 | ❌ | ❌ |
| coretto-21.0.1.12 | ❌ | ❌ |
AFAIK java.io.Reader#reset has never been synchronized, so that one is easy. For java.io.InputStream#reset, I guess it should be synchronized to line up with Java 8, since that's the library's targeted version?
There was a problem hiding this comment.
Well, this PR is about a new feature and should not flip-flop an unrelated implementation detail IMO. Especially when the setting seems to change between vendo and version.
There was a problem hiding this comment.
FWIW, I see synchronized on InputStream Temurin 17 but not Temurin 21.
src/main/java/org/apache/commons/io/input/BrokenInputStream.java
Outdated
Show resolved
Hide resolved
src/test/java/org/apache/commons/io/input/BrokenInputStreamTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/apache/commons/io/input/BrokenReaderTest.java
Outdated
Show resolved
Hide resolved
| @@ -117,10 +117,10 @@ public int read() throws IOException { | |||
| /** | |||
| * Throws the configured exception. | |||
| * | |||
| * @throws IOException always thrown | |||
| * @throws IOException always throws the exception configured in the constructor. | |||
There was a problem hiding this comment.
Since there is more than one constructor, I think we should say "in a constructor", not in "in the...".
There was a problem hiding this comment.
As you like - I read this as 'in the constructor [you callled]', but I don't have a strong feeling either way.
There was a problem hiding this comment.
How about "always throws the exception configured on construction"?
There was a problem hiding this comment.
How about "always throws the exception configured on construction"?
I'm not going to go round and round on such a small point. I'll likely review post merge and resolve whatever is left.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #530 +/- ##
============================================
- Coverage 86.17% 86.01% -0.17%
+ Complexity 3433 3418 -15
============================================
Files 229 229
Lines 8162 8167 +5
Branches 959 959
============================================
- Hits 7034 7025 -9
- Misses 845 860 +15
+ Partials 283 282 -1 ☔ View full report in Codecov by Sentry. |
…t / BrokenOutputStreamTest / BrokenWriterTest.
garydgregory
changed the title
- Add BrokenReader.BrokenReader(Throwable) - Add BrokenOutputStream.BrokenOutputStream - Add BrokenWriter.BrokenWriter(Throwable)
|
@markslater PR merged, TY! |
markslater
deleted the
add-runtime-exception-support-to-broken-streams-erase-implementation
branch
https://issues.apache.org/jira/browse/IO-826
As discussed in #528.
One point to note: Is there a reason to keep the deprecated constructors such as
BrokenInputStream(final IOException exception)? Given that the new constructors such asBrokenInputStream(final Throwable exception)accept a superclass ofIOException, it seems the old constructor could be removed without a breaking change.