Skip to content

Limit write chunk size in IOSessionImpl - #700

Merged
ok2c merged 1 commit into
apache:masterfrom
yhzdys:fix
Sep 25, 2026
Merged

ok2c merged 1 commit into
apache:masterfrom
yhzdys:fix

Conversation

@yhzdys

@yhzdys yhzdys commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Summary

IOSessionImpl#write(ByteBuffer) forwards the entire source buffer to the socket channel in a single write operation.
When a heap ByteBuffer is written through a socket channel, the JDK copies its content through a temporary direct buffer of the same size.
These buffers are cached per I/O dispatcher thread and never released for the entire thread life-span,
so large buffered output can pin an equally large amount of native memory per dispatcher thread — invisible to the GC, not bounded by -Xmx,
and able to trigger OutOfMemoryError: Direct buffer memory under concurrent large transfers.

Change

Chunk heap writes to 16 KB per underlying channel write operation, temporarily narrowing the buffer limit and restoring it in a finally block.
IOSessionImpl#write is the single choke point for socket writes, so this covers all transports (HTTP/1.1, HTTP/2, TLS).
Direct buffers bypass the chunking as the JDK does not copy them.
The chunk size is hardwired for now; it matches the default HTTP/2 max frame size and the internal buffer size used by the JDK's built-in HTTP client.

@yhzdys
yhzdys force-pushed the fix branch 2 times, most recently from 1a7e393 to 050a56e Compare September 23, 2026 03:34
@arturobernalg

Copy link
Copy Markdown
Member

I do not think BasicAsyncEntityProducer is the right place to address this problem. Entity producers should not be concerned with JDK socket implementation details or HTTP/2 frame sizes.

I think should be in IOSessionImpl.write(ByteBuffer) or similar.

Why 16K?

@yhzdys

yhzdys commented Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

I do not think BasicAsyncEntityProducer is the right place to address this problem. Entity producers should not be concerned with JDK socket implementation details or HTTP/2 frame sizes.

I think should be in IOSessionImpl.write(ByteBuffer) or similar.

Why 16K?

Agreed — this belongs in the transport layer (IOSessionImpl#write(ByteBuffer) or similar), not in entity producers. I'll rework it there.

The 16K choice was not a hard requirement — just a reasonable upper bound: it matches the default HTTP/2 max frame size and the JDK HTTP client's internal buffer size,
so it amortizes syscalls while keeping the temporary direct buffer small.
At the transport layer it can simply be a constant or a configurable setting; happy to go with whichever you prefer.

@ok2c

ok2c commented Sep 23, 2026

Copy link
Copy Markdown
Member

@yhzdys I fully agree with @arturobernalg The i/o session should be the right place to address the issue. If you could look into it, it would be awesome. You can start with 16K hardwired. We can figure out at the later point the best way to make it configurable

@yhzdys

yhzdys commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor Author

@yhzdys I fully agree with @arturobernalg The i/o session should be the right place to address the issue. If you could look into it, it would be awesome. You can start with 16K hardwired. We can figure out at the later point the best way to make it configurable

Thanks! I'll rework it.

@yhzdys yhzdys changed the title Limit write chunk size in BasicAsyncEntityProducer Limit write chunk size in IOSessionImpl Sep 24, 2026
@yhzdys

yhzdys commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

@arturobernalg Reworked as suggested — the fix is now in IOSessionImpl#write(ByteBuffer) with a hardwired 16K constant for now. Direct buffers bypass the chunking. This covers all transports since they all funnel output through IOSession#write.

@arturobernalg arturobernalg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me.

@ok2c ok2c left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@yhzdys Awesome.

@ok2c
ok2c merged commit 113e78d into apache:master Sep 25, 2026
12 checks passed
@yhzdys
yhzdys deleted the fix branch October 9, 2026 05:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants