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.

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

Jump to bottom

Make static final buffers unreleasable and read-only #11802

Merged
merged 6 commits into from Nov 3, 2021
Merged

Make static final buffers unreleasable and read-only #11802

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4a8a855
Make all static final buffer read-only
alexc-db Oct 27, 2021
4f3cf4c
Revert pom.xml changes to let PR checks run
alexc-db Oct 27, 2021
2e301dd
add missing .duplicate() calls
alexc-db Oct 27, 2021
977db3b
Revert change for Unpooled.EMPTY_BUFFER - EmptyByteBuf instances are …
alexc-db Oct 27, 2021
bde5357
Merge remote-tracking branch 'netty/4.1' into read-only-buffers
alexc-db Nov 1, 2021
644804d
Bump netty-build version
alexc-db Nov 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions buffer/src/main/java/io/netty/buffer/Unpooled.java
View file
Open in desktop
Expand Up @@ -87,6 +87,7 @@ public final class Unpooled {
/**
* A buffer whose capacity is {@code 0}.
*/
@SuppressWarnings("checkstyle:StaticFinalBuffer") // EmptyByteBuf is not writeable or readable.
public static final ByteBuf EMPTY_BUFFER = ALLOC.buffer(0, 0);
Copy link
Member

Choose a reason for hiding this comment

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

I guess we could also just call asReadOnly() and remove the SupressWarnings ?

Copy link
Member

Choose a reason for hiding this comment

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

Also we should ensure to wrap it in unreleasableBuffer to make it more "future proof"

Copy link
Contributor Author

@alexc-db alexc-db Oct 29, 2021 •
edited

Choose a reason for hiding this comment

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

That'd be an incompatible public API change. EMPTY_BUFFER is an instance of EmptyByteBuf (see static assert below). asReadOnly wraps instance into the ReadOnlyByteBuf (and unreleasableBuffer wraps in the UnreleasableByteBuf). If instance type is changed it'll break any downstream dependency that does instanceof EmptyByteBuf check.

What you suggested was my initial implementation, but the assert below and EmptyByteBuf API guarantees that it's not readable nor writable. Functionally, asReadOnly is not needed here, and SuppressWarnings + comment should be sufficient to document the expected behavior.

Let me know if I misses something, or if you think comment should be more detailed.

Copy link
Member

Choose a reason for hiding this comment

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

got it... ok yeah its ok then :)


static {
Expand Down
7 changes: 4 additions & 3 deletions codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectEncoder.java
View file
Open in desktop
Expand Up @@ -50,9 +50,10 @@ public abstract class HttpObjectEncoder<H extends HttpMessage> extends MessageTo
static final int CRLF_SHORT = (CR << 8) | LF;
private static final int ZERO_CRLF_MEDIUM = ('0' << 16) | CRLF_SHORT;
private static final byte[] ZERO_CRLF_CRLF = { '0', CR, LF, CR, LF };
private static final ByteBuf CRLF_BUF = unreleasableBuffer(directBuffer(2).writeByte(CR).writeByte(LF));
private static final ByteBuf ZERO_CRLF_CRLF_BUF = unreleasableBuffer(directBuffer(ZERO_CRLF_CRLF.length)
.writeBytes(ZERO_CRLF_CRLF));
private static final ByteBuf CRLF_BUF = unreleasableBuffer(
directBuffer(2).writeByte(CR).writeByte(LF)).asReadOnly();
private static final ByteBuf ZERO_CRLF_CRLF_BUF = unreleasableBuffer(
directBuffer(ZERO_CRLF_CRLF.length).writeBytes(ZERO_CRLF_CRLF)).asReadOnly();
private static final float HEADERS_WEIGHT_NEW = 1 / 5f;
private static final float HEADERS_WEIGHT_HISTORICAL = 1 - HEADERS_WEIGHT_NEW;
private static final float TRAILERS_WEIGHT_NEW = HEADERS_WEIGHT_NEW;
Expand Down
6 changes: 3 additions & 3 deletions codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocket00FrameEncoder.java
View file
Open in desktop
Expand Up @@ -32,11 +32,11 @@
@Sharable
public class WebSocket00FrameEncoder extends MessageToMessageEncoder<WebSocketFrame> implements WebSocketFrameEncoder {
private static final ByteBuf _0X00 = Unpooled.unreleasableBuffer(
Unpooled.directBuffer(1, 1).writeByte((byte) 0x00));
Unpooled.directBuffer(1, 1).writeByte((byte) 0x00)).asReadOnly();
private static final ByteBuf _0XFF = Unpooled.unreleasableBuffer(
Unpooled.directBuffer(1, 1).writeByte((byte) 0xFF));
Unpooled.directBuffer(1, 1).writeByte((byte) 0xFF)).asReadOnly();
private static final ByteBuf _0XFF_0X00 = Unpooled.unreleasableBuffer(
Unpooled.directBuffer(2, 2).writeByte((byte) 0xFF).writeByte((byte) 0x00));
Unpooled.directBuffer(2, 2).writeByte((byte) 0xFF).writeByte((byte) 0x00)).asReadOnly();

@Override
protected void encode(ChannelHandlerContext ctx, WebSocketFrame msg, List<Object> out) throws Exception {
Expand Down
2 changes: 1 addition & 1 deletion ...-http2/src/main/java/io/netty/handler/codec/http2/CleartextHttp2ServerUpgradeHandler.java
View file
Open in desktop
Expand Up @@ -39,7 +39,7 @@
*/
@UnstableApi
public final class CleartextHttp2ServerUpgradeHandler extends ByteToMessageDecoder {
private static final ByteBuf CONNECTION_PREFACE = unreleasableBuffer(connectionPrefaceBuf());
private static final ByteBuf CONNECTION_PREFACE = unreleasableBuffer(connectionPrefaceBuf()).asReadOnly();

private final HttpServerCodec httpServerCodec;
private final HttpServerUpgradeHandler httpServerUpgradeHandler;
Expand Down
2 changes: 1 addition & 1 deletion codec-smtp/src/main/java/io/netty/handler/codec/smtp/SmtpRequestEncoder.java
View file
Open in desktop
Expand Up @@ -34,7 +34,7 @@ public final class SmtpRequestEncoder extends MessageToMessageEncoder<Object> {
private static final int CRLF_SHORT = ('\r' << 8) | '\n';
private static final byte SP = ' ';
private static final ByteBuf DOT_CRLF_BUFFER = Unpooled.unreleasableBuffer(
Unpooled.directBuffer(3).writeByte('.').writeByte('\r').writeByte('\n'));
Unpooled.directBuffer(3).writeByte('.').writeByte('\r').writeByte('\n')).asReadOnly();

private boolean contentExpected;

Expand Down
1 change: 1 addition & 0 deletions codec/src/main/java/io/netty/handler/codec/ReplayingDecoderByteBuf.java
View file
Open in desktop
Expand Up @@ -44,6 +44,7 @@ final class ReplayingDecoderByteBuf extends ByteBuf {
private boolean terminated;
private SwappedByteBuf swapped;

@SuppressWarnings("checkstyle:StaticFinalBuffer") // Unpooled.EMPTY_BUFFER is not writeable or readable.
static final ReplayingDecoderByteBuf EMPTY_BUFFER = new ReplayingDecoderByteBuf(Unpooled.EMPTY_BUFFER);
Copy link
Member

Choose a reason for hiding this comment

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

See above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ditto #11802 (comment) - it's a package-private static variable that wraps an instance of EmptyByteBuf, asReadOnly would change the type of the wrapped instance.

Though I wasn't able to find usages, maybe this one can be removed? Or did I miss it?


static {
Expand Down
17 changes: 7 additions & 10 deletions codec/src/test/java/io/netty/handler/codec/compression/AbstractDecoderTest.java
View file
Open in desktop
Expand Up @@ -31,13 +31,10 @@
@TestInstance(TestInstance.Lifecycle.PER_CLASS)
public abstract class AbstractDecoderTest extends AbstractCompressionTest {

protected static final ByteBuf WRAPPED_BYTES_SMALL;
protected static final ByteBuf WRAPPED_BYTES_LARGE;

static {
WRAPPED_BYTES_SMALL = Unpooled.wrappedBuffer(BYTES_SMALL);
WRAPPED_BYTES_LARGE = Unpooled.wrappedBuffer(BYTES_LARGE);
}
protected static final ByteBuf WRAPPED_BYTES_SMALL = Unpooled.unreleasableBuffer(
Unpooled.wrappedBuffer(BYTES_SMALL)).asReadOnly();
protected static final ByteBuf WRAPPED_BYTES_LARGE = Unpooled.unreleasableBuffer(
Unpooled.wrappedBuffer(BYTES_LARGE)).asReadOnly();

protected EmbeddedChannel channel;

Expand Down Expand Up @@ -86,19 +83,19 @@ public ByteBuf[] largeData() {
@ParameterizedTest
@MethodSource("smallData")
public void testDecompressionOfSmallChunkOfData(ByteBuf data) throws Exception {
testDecompression(WRAPPED_BYTES_SMALL, data);
testDecompression(WRAPPED_BYTES_SMALL.duplicate(), data);
}

@ParameterizedTest
@MethodSource("largeData")
public void testDecompressionOfLargeChunkOfData(ByteBuf data) throws Exception {
testDecompression(WRAPPED_BYTES_LARGE, data);
testDecompression(WRAPPED_BYTES_LARGE.duplicate(), data);
}

@ParameterizedTest
@MethodSource("largeData")
public void testDecompressionOfBatchedFlowOfData(ByteBuf data) throws Exception {
testDecompressionOfBatchedFlow(WRAPPED_BYTES_LARGE, data);
testDecompressionOfBatchedFlow(WRAPPED_BYTES_LARGE.duplicate(), data);
}

protected void testDecompression(final ByteBuf expected, final ByteBuf data) throws Exception {
Expand Down
13 changes: 7 additions & 6 deletions codec/src/test/java/io/netty/handler/codec/compression/BrotliDecoderTest.java
View file
Open in desktop
Expand Up @@ -41,8 +41,6 @@ public class BrotliDecoderTest {
private static final Random RANDOM;
private static final byte[] BYTES_SMALL = new byte[256];
private static final byte[] BYTES_LARGE = new byte[256 * 1024];
private static final ByteBuf WRAPPED_BYTES_SMALL;
private static final ByteBuf WRAPPED_BYTES_LARGE;
private static final byte[] COMPRESSED_BYTES_SMALL;
private static final byte[] COMPRESSED_BYTES_LARGE;

Expand All @@ -52,15 +50,18 @@ public class BrotliDecoderTest {
RANDOM = new Random();
fillArrayWithCompressibleData(BYTES_SMALL);
fillArrayWithCompressibleData(BYTES_LARGE);
WRAPPED_BYTES_SMALL = Unpooled.wrappedBuffer(BYTES_SMALL);
WRAPPED_BYTES_LARGE = Unpooled.wrappedBuffer(BYTES_LARGE);
COMPRESSED_BYTES_SMALL = compress(BYTES_SMALL);
COMPRESSED_BYTES_LARGE = compress(BYTES_LARGE);
} catch (Throwable throwable) {
throw new ExceptionInInitializerError(throwable);
}
}

private static final ByteBuf WRAPPED_BYTES_SMALL = Unpooled.unreleasableBuffer(
Unpooled.wrappedBuffer(BYTES_SMALL)).asReadOnly();
private static final ByteBuf WRAPPED_BYTES_LARGE = Unpooled.unreleasableBuffer(
Unpooled.wrappedBuffer(BYTES_LARGE)).asReadOnly();

static boolean isNotSupported() {
return PlatformDependent.isOsx() && "aarch_64".equals(PlatformDependent.normalizedArch());
}
Expand Down Expand Up @@ -111,13 +112,13 @@ public static ByteBuf[] largeData() {
@ParameterizedTest
@MethodSource("smallData")
public void testDecompressionOfSmallChunkOfData(ByteBuf data) {
testDecompression(WRAPPED_BYTES_SMALL, data);
testDecompression(WRAPPED_BYTES_SMALL.duplicate(), data);
}

@ParameterizedTest
@MethodSource("largeData")
public void testDecompressionOfLargeChunkOfData(ByteBuf data) {
testDecompression(WRAPPED_BYTES_LARGE, data);
testDecompression(WRAPPED_BYTES_LARGE.duplicate(), data);
}

@ParameterizedTest
Expand Down
3 changes: 2 additions & 1 deletion .../src/main/java/io/netty/example/http2/helloworld/frame/server/HelloWorldHttp2Handler.java
View file
Open in desktop
Expand Up @@ -42,7 +42,8 @@
@Sharable
public class HelloWorldHttp2Handler extends ChannelDuplexHandler {

static final ByteBuf RESPONSE_BYTES = unreleasableBuffer(copiedBuffer("Hello World", CharsetUtil.UTF_8));
static final ByteBuf RESPONSE_BYTES = unreleasableBuffer(
copiedBuffer("Hello World", CharsetUtil.UTF_8)).asReadOnly();

@Override
public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws Exception {
Expand Down
3 changes: 2 additions & 1 deletion .../main/java/io/netty/example/http2/helloworld/multiplex/server/HelloWorldHttp2Handler.java
View file
Open in desktop
Expand Up @@ -40,7 +40,8 @@
@Sharable
public class HelloWorldHttp2Handler extends ChannelDuplexHandler {

static final ByteBuf RESPONSE_BYTES = unreleasableBuffer(copiedBuffer("Hello World", CharsetUtil.UTF_8));
static final ByteBuf RESPONSE_BYTES = unreleasableBuffer(
copiedBuffer("Hello World", CharsetUtil.UTF_8)).asReadOnly();

@Override
public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws Exception {
Expand Down
3 changes: 2 additions & 1 deletion example/src/main/java/io/netty/example/http2/helloworld/server/HelloWorldHttp2Handler.java
View file
Open in desktop
Expand Up @@ -42,7 +42,8 @@
*/
public final class HelloWorldHttp2Handler extends Http2ConnectionHandler implements Http2FrameListener {

static final ByteBuf RESPONSE_BYTES = unreleasableBuffer(copiedBuffer("Hello World", CharsetUtil.UTF_8));
static final ByteBuf RESPONSE_BYTES = unreleasableBuffer(
copiedBuffer("Hello World", CharsetUtil.UTF_8)).asReadOnly();

HelloWorldHttp2Handler(Http2ConnectionDecoder decoder, Http2ConnectionEncoder encoder,
Http2Settings initialSettings) {
Expand Down
2 changes: 1 addition & 1 deletion pom.xml
View file
Open in desktop
Expand Up @@ -484,7 +484,7 @@
<netty.dev.tools.directory>${project.build.directory}/dev-tools</netty.dev.tools.directory>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding>
<netty.build.version>29</netty.build.version>
<netty.build.version>30</netty.build.version>
<jboss.marshalling.version>1.4.11.Final</jboss.marshalling.version>
<jetty.alpnAgent.version>2.0.10</jetty.alpnAgent.version>
<jetty.alpnAgent.path>"${settings.localRepository}"/org/mortbay/jetty/alpn/jetty-alpn-agent/${jetty.alpnAgent.version}/jetty-alpn-agent-${jetty.alpnAgent.version}.jar</jetty.alpnAgent.path>
Expand Down
3 changes: 2 additions & 1 deletion testsuite-http2/src/main/java/io/netty/testsuite/http2/HelloWorldHttp2Handler.java
View file
Open in desktop
Expand Up @@ -42,7 +42,8 @@
*/
public final class HelloWorldHttp2Handler extends Http2ConnectionHandler implements Http2FrameListener {

static final ByteBuf RESPONSE_BYTES = unreleasableBuffer(copiedBuffer("Hello World", CharsetUtil.UTF_8));
static final ByteBuf RESPONSE_BYTES = unreleasableBuffer(
copiedBuffer("Hello World", CharsetUtil.UTF_8)).asReadOnly();

HelloWorldHttp2Handler(Http2ConnectionDecoder decoder, Http2ConnectionEncoder encoder,
Http2Settings initialSettings) {
Expand Down