Skip to content

Commit d21261e

Browse files
rschmittok2c
authored andcommitted
SSLIOSession: Fix regression in backpressure handling
A regression was introduced by PR #513, submitted as a fix for HTTPCORE-775, which claimed that `SSLIOSession::write` was failing to "handle" `SSLEngineResult#BUFFER_OVERFLOW`. The issue was encountered by Apache CXF, who provided a test case demonstrating the problem: https://github.com/apache/cxf/pull/2214/changes I investigated this reproducer and found that CXF was actually hanging due to not signaling the availability of more data: https://github.com/apache/cxf/blob/f1b2c37bd9f3606cf7ca2d5d39cc1168e94fd9e4/rt/transports/http-hc5/src/main/java/org/apache/cxf/transport/http/asyncclient/hc5/CXFHttpAsyncRequestProducer.java#L97 when `SSLIOSession#write` signaled backpressure here: https://github.com/apache/cxf/blob/f1b2c37bd9f3606cf7ca2d5d39cc1168e94fd9e4/rt/transports/http-hc5/src/main/java/org/apache/cxf/transport/http/asyncclient/hc5/CXFHttpAsyncRequestProducer.java#L75 The return value of `buf.produceContent` (which is ultimately `SSLEngineResult#bytesConsumed` -- 0, in this case) is simply being discarded. Changing the `CXFHttpAsyncRequestProducer#available` method as follows causes the problematic test cases to succeed instantly: ``` @OverRide public int available() { if (buffer != null && buffer.hasRemaining()) { return buffer.remaining(); } else if (buf != null && buf.length() > 0) { return buf.length(); } return 0; } ``` Additionally, that PR appears to have introduced dysfunctional backpressure handling into `SSLIOSession` by simply expanding the buffer as much as necessary to hold the encrypted `src` buffer. This in turn has caused a significant regression in heap usage and latency, hence the need to revert. I've retained the test changes, since they did introduce more coverage for TLSv1.3.
1 parent fdc53a3 commit d21261e

3 files changed

Lines changed: 10 additions & 66 deletions

File tree

httpcore5-testing/src/test/java/org/apache/hc/core5/testing/nio/Http1IntegrationTest.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -750,6 +750,9 @@ public void complete(final List<? extends Header> trailers) throws IOException {
750750

751751
@Override
752752
public int write(final ByteBuffer src) throws IOException {
753+
if (!channel().isOpen()) {
754+
return 0;
755+
}
753756
final int chunk;
754757
if (!done) {
755758
lineBuffer.clear();

httpcore5/src/main/java/org/apache/hc/core5/reactor/ssl/SSLIOSession.java

Lines changed: 6 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,6 @@ enum TLSHandShakeState { READY, INITIALIZED, HANDSHAKING, COMPLETE }
9393
private final AtomicInteger outboundClosedCount;
9494
private final AtomicReference<TLSHandShakeState> handshakeStateRef;
9595
private final IOEventHandler internalEventHandler;
96-
private final int packetBufferSize;
9796

9897
private int appEventMask;
9998

@@ -180,9 +179,9 @@ public SSLIOSession(
180179

181180
final SSLSession sslSession = this.sslEngine.getSession();
182181
// Allocate buffers for network (encrypted) data
183-
this.packetBufferSize = sslSession.getPacketBufferSize();
184-
this.inEncrypted = SSLManagedBuffer.create(sslBufferMode, packetBufferSize);
185-
this.outEncrypted = SSLManagedBuffer.create(sslBufferMode, packetBufferSize);
182+
final int netBufferSize = sslSession.getPacketBufferSize();
183+
this.inEncrypted = SSLManagedBuffer.create(sslBufferMode, netBufferSize);
184+
this.outEncrypted = SSLManagedBuffer.create(sslBufferMode, netBufferSize);
186185

187186
// Allocate buffers for application (unencrypted) data
188187
final int appBufferSize = sslSession.getApplicationBufferSize();
@@ -670,18 +669,9 @@ public int write(final ByteBuffer src) throws IOException {
670669
if (this.handshakeStateRef.get() == TLSHandShakeState.READY) {
671670
return 0;
672671
}
673-
674-
for (;;) {
675-
final ByteBuffer outEncryptedBuf = this.outEncrypted.acquire();
676-
final SSLEngineResult result = doWrap(src, outEncryptedBuf);
677-
if (result.getStatus() == SSLEngineResult.Status.BUFFER_OVERFLOW) {
678-
// We don't release the buffer here, it will be expanded (if needed)
679-
// and returned by the next attempt of SSLManagedBuffer#acquire() call.
680-
this.outEncrypted.ensureWriteable(packetBufferSize);
681-
} else {
682-
return result.bytesConsumed();
683-
}
684-
}
672+
final ByteBuffer outEncryptedBuf = this.outEncrypted.acquire();
673+
final SSLEngineResult result = doWrap(src, outEncryptedBuf);
674+
return result.bytesConsumed();
685675
} finally {
686676
this.session.getLock().unlock();
687677
}

httpcore5/src/main/java/org/apache/hc/core5/reactor/ssl/SSLManagedBuffer.java

Lines changed: 1 addition & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -57,54 +57,13 @@ abstract class SSLManagedBuffer {
5757
*/
5858
abstract boolean hasData();
5959

60-
/**
61-
* Expands the underlying buffer's to make sure it has enough write capacity to accommodate
62-
* the required amount of bytes. This method has no side effect if the buffer has enough writeable
63-
* capacity left.
64-
* @param size the required write capacity
65-
*/
66-
abstract void ensureWriteable(final int size);
67-
68-
/**
69-
* Helper method to ensure additional writeable capacity with respect to the source buffer. It
70-
* allocates a new buffer and copies all the data if needed, returning the new buffer. This method
71-
* has no side effect if the source buffer has enough writeable capacity left.
72-
* @param src source buffer
73-
* @param size the required write capacity
74-
* @return new buffer (or the source buffer of it has enough writeable capacity left)
75-
*/
76-
ByteBuffer ensureWriteable(final ByteBuffer src, final int size) {
77-
if (src == null) {
78-
// Nothing to do, the buffer is not allocated
79-
return null;
80-
}
81-
82-
// There is not enough capacity left, we need to expand
83-
if (src.remaining() < size) {
84-
final int additionalCapacityNeeded = size - src.remaining();
85-
final ByteBuffer expanded = ByteBuffer.allocate(src.capacity() + additionalCapacityNeeded);
86-
87-
// use a duplicated buffer so we don't disrupt the limit of the original buffer
88-
final ByteBuffer tmp = src.duplicate();
89-
tmp.flip();
90-
91-
// Copy to expanded buffer
92-
expanded.put(tmp);
93-
94-
// Use a new buffer
95-
return expanded;
96-
} else {
97-
return src;
98-
}
99-
}
100-
10160
static SSLManagedBuffer create(final SSLBufferMode mode, final int size) {
10261
return mode == SSLBufferMode.DYNAMIC ? new DynamicBuffer(size) : new StaticBuffer(size);
10362
}
10463

10564
static final class StaticBuffer extends SSLManagedBuffer {
10665

107-
private ByteBuffer buffer;
66+
private final ByteBuffer buffer;
10867

10968
public StaticBuffer(final int size) {
11069
Args.positive(size, "size");
@@ -131,10 +90,6 @@ public boolean hasData() {
13190
return buffer.position() > 0;
13291
}
13392

134-
@Override
135-
void ensureWriteable(final int size) {
136-
buffer = ensureWriteable(buffer, size);
137-
}
13893
}
13994

14095
static final class DynamicBuffer extends SSLManagedBuffer {
@@ -171,10 +126,6 @@ public boolean hasData() {
171126
return wrapped != null && wrapped.position() > 0;
172127
}
173128

174-
@Override
175-
void ensureWriteable(final int size) {
176-
wrapped = ensureWriteable(wrapped, size);
177-
}
178129
}
179130

180131
}

0 commit comments

Comments
 (0)