Skip to content

BaseEncoding: Make encodingStream().close() idempotent #8183

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.

Sign up for GitHub

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

Open
nickita-khylkouski wants to merge 1 commit into
base: master
Choose a base branch
from
+44 −3
Open

BaseEncoding: Make encodingStream().close() idempotent #8183

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions guava-tests/test/com/google/common/io/BaseEncodingTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -572,6 +572,34 @@ private static void testStreamingDecodes(BaseEncoding encoding, String encoded,
}
}

@GwtIncompatible // Writer,OutputStream
public void testEncodingStreamCloseIsIdempotent() throws IOException {
StringWriter writer = new StringWriter();
OutputStream encodingStream = base64().encodingStream(writer);
encodingStream.write(0);
encodingStream.close();
assertThat(writer.toString()).isEqualTo("AA==");
// Closing again should have no effect.
encodingStream.close();
assertThat(writer.toString()).isEqualTo("AA==");
}

@GwtIncompatible // Writer,OutputStream
public void testEncodingStreamWriteAfterClose() throws IOException {
StringWriter writer = new StringWriter();
OutputStream encodingStream = base64().encodingStream(writer);
encodingStream.close();
assertThrows(IOException.class, () -> encodingStream.write(0));
}

@GwtIncompatible // Writer,OutputStream
public void testEncodingStreamFlushAfterClose() throws IOException {
StringWriter writer = new StringWriter();
OutputStream encodingStream = base64().encodingStream(writer);
encodingStream.close();
assertThrows(IOException.class, () -> encodingStream.flush());
}

public void testToString() {
assertThat(base64().toString()).isEqualTo("BaseEncoding.base64().withPadChar('=')");
assertThat(base32Hex().omitPadding().toString())
Expand Down
2 changes: 2 additions & 0 deletions guava-tests/test/com/google/common/io/ReflectionFreeAssertThrows.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import com.google.common.base.VerifyException;
import com.google.common.collect.ImmutableMap;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
import java.io.IOException;
import java.lang.reflect.InvocationTargetException;
import java.nio.charset.UnsupportedCharsetException;
import java.util.ConcurrentModificationException;
Expand Down Expand Up @@ -136,6 +137,7 @@ ImmutableMap<Class<? extends Throwable>, Predicate<Throwable>> exceptions() {
.put(ExecutionException.class, e -> e instanceof ExecutionException)
.put(IllegalArgumentException.class, e -> e instanceof IllegalArgumentException)
.put(IllegalStateException.class, e -> e instanceof IllegalStateException)
.put(IOException.class, e -> e instanceof IOException)
.put(IndexOutOfBoundsException.class, e -> e instanceof IndexOutOfBoundsException)
.put(NoSuchElementException.class, e -> e instanceof NoSuchElementException)
.put(NullPointerException.class, e -> e instanceof NullPointerException)
Expand Down
17 changes: 14 additions & 3 deletions guava/src/com/google/common/io/BaseEncoding.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -645,9 +645,13 @@ public OutputStream encodingStream(Writer out) {
int bitBuffer = 0;
int bitBufferLength = 0;
int writtenChars = 0;
boolean closed = false;

@Override
public void write(int b) throws IOException {
public synchronized void write(int b) throws IOException {
Copy link
Copy Markdown

@softpapers softpapers Feb 7, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Synchronization is required for reliable communication between threads as well as for mutual exclusion.

  2. Synchronization is not guaranteed to work unless both read and write operations are synchronized.

"Occasionally a program that synchronizes only writes (or reads) may appear to work on some machines, but in this case, appearances are deceiving."

--- Item 78 of Effective Java Programming by Joshua Blosch.

What justifies the need for the write of an abstract class to be synchronised without synchronising the read?

Copy link
Copy Markdown
Contributor Author

@nickita-khylkouski nickita-khylkouski Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All three methods that access the closed field — write(), flush(), and close() — are synchronized. Both the write to closed (in close()) and the reads of closed (in write() and flush()) happen within synchronized methods. There is no unsynchronized read.

This matches CharSequenceReader, which synchronizes every method. The synchronization was added per @cpovirk's guidance to ensure close() and write() cannot interleave.

if (closed) {
throw new IOException("Stream is closed");
Copy link
Copy Markdown

@softpapers softpapers Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Steam is closed" is repeated twice. Shouldn't that be defined as a reusable constant?

Copy link
Copy Markdown
Contributor Author

@nickita-khylkouski nickita-khylkouski Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Guava pattern for this is a checkNotClosed() helper method, not a string constant — see AppendableWriter. Since this is a small anonymous class with only two call sites, I kept it inline, but happy to extract a helper if maintainers prefer.

Copy link
Copy Markdown

@softpapers softpapers Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough.

}
bitBuffer <<= 8;
bitBuffer |= b & 0xFF;
bitBufferLength += 8;
Expand All @@ -660,12 +664,19 @@ public void write(int b) throws IOException {
}

@Override
public void flush() throws IOException {
public synchronized void flush() throws IOException {
if (closed) {
throw new IOException("Stream is closed");
}
out.flush();
}

@Override
public void close() throws IOException {
public synchronized void close() throws IOException {
if (closed) {
return;
}
closed = true;
if (bitBufferLength > 0) {
int charIndex = (bitBuffer << (alphabet.bitsPerChar - bitBufferLength)) & alphabet.mask;
out.write(alphabet.encode(charIndex));
Expand Down
Loading