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

Messages over 1MiB stop all communication for a NIO UDS #118

Closed
TW-Goldencode opened this issue Sep 28, 2022 · 9 comments
Closed

Messages over 1MiB stop all communication for a NIO UDS #118

TW-Goldencode opened this issue Sep 28, 2022 · 9 comments
Labels
bug The issue describes a bug in the code verify The issue is considered fixed/done, and reassigned to the originator to verify.
Milestone
2.5.2

Comments

@TW-Goldencode
Copy link

Describe the bug:
Sending a message larger than 1MiB from server to client results in the following infinite loop:

  1. The client blocks at read()
  2. The server can call write(), but it keeps returning 0 , while the transferred byte count stays at exactly 1 * 1024 * 1024 bytes

Environment:

  • OS: Linux
  • Distribution: Ubuntu
  • Version: 22.04
  • junixsocket 2.5.1 (latest, gradle, java 8) (the selftest and build tests run fine, also on a custom build)

Fix:
The cause is: DATAGRAMPACKET_BUFFER_MAX_CAPACITY in https://github.com/kohlschutter/junixsocket/blob/main/junixsocket-common/src/main/java/org/newsclub/net/unix/AFCore.java is in effect.
This seems strange, since it's not a datagram at all.
The OS has plenty of protection via sysctl net.core.wmem_max and sysctl net.core.rmem_max , and within this limits can be tuned by (example) this.channel.setOption(java.net.StandardSocketOptions.SO_SNDBUF, 8388608); and this.channel.setOption(java.net.StandardSocketOptions.SO_RCVBUF, 8388608);

I did a custom build which removed the artificial hardcoded limit. After that it works fine.

Please indicate if you prefer a pull request for your fine library.
I'd suggest using a system variable, if set to 0 meaning unlimited.
Do you have other ideas?
kohlschuetter added a commit that referenced this issue Sep 28, 2022
@kohlschuetter
AFCore: Raise thread-local buffer capacity limit to 8 MB, add property
478ea2b 
We currently use ThreadLocal-defined direct byte buffers to allow
callers to use non-direct buffers where we really need direct ones.

The current maximum limit is 1MB, which breaks support for larger
datagrams.

Raise the limit from 1 MB to 8 MB, and allow configuration via a system
property, org.newsclub.net.unix.thread-local-buffer.max-capacity, which
takes the maximum capacity in bytes, or 0 for "unlimited".

Using 0 is highly discouraged, as it may effectively block large chunks
of memory.

#118
@kohlschuetter
Copy link
Member

Hi @TW-Goldencode, thanks for reporting!

Please try the above commit. Let me know if you actually managed to get datagrams larger than 8 MB, or if that really is a good upper limit. Out of curiosity, can you explain for what you need these humungous datagrams, and how they perform compared to smaller ones?

Cheers,
Christian

@TW-Goldencode
Copy link
Author

@kohlschuetter Fantastic, thank you for this fix.
"humungous" : I agree, messages of this size should ideally be split. It performs fine, but unlimited size can lead to unexpected memory issues.
We detected this issue on an internal IPC where we can control both client and server. For that specific scenario, we can and will split it up.
It still got us worried for situations outside of our control (when we IPC an external service like Postgresql or Redis).
Your fix will address that.
One thing might be improved : a warning, raise, and/or abend if the limit is reached. Now it just 'hangs'. But don't address this on our account, the fix is good as-is.
I'll do a test next Friday and will get back to you.
It would be great if you could create a release for this.

@TW-Goldencode
Copy link
Author

@kohlschuetter Tested, it's all good. Would it be possible to create a 2.5.2 release for this?

Side notes: During the bootstrap, messages with a max of around 25MiB were transmitted. The size is not capped. The only safe way for us right now is -Dorg.newsclub.net.unix.thread-local-buffer.max-capacity=0 . That works good in the patch, as expected. I applied the patch on top of 2.5.1, and there were no side effects for the build.

Thanks again, we'll wait for release 2.5.2

kohlschuetter added a commit that referenced this issue Sep 30, 2022
@kohlschuetter
AFCore: Revert limit to 1MB; allocate buffers on-the-fly if exceeded
bf9fb50 
The limit to how large datagram can be seems to have no feasibly low
limit (25MB datagrams have been reported to work). This imposes a
challenge on caching/reuse strategies for direct byte buffers (a shared,
reusable pool that is not thread-specific could be an alternative, but
comes at the cost of complexity).

At the cost of performance, revert the per-thread limit to 1MB, and
return newly allocated direct byte buffers instead of cached ones
whenever the limit is exceeded.

Users of such unexpectedly large datagrams could either still force a
higher (or unbounded) limit via the system property
"org.newsclub.net.unix.thread-local-buffer.max-capacity", or better,
use direct byte buffers in the calling code, obsoleting the need to
use this cache in the first place.

#118
@kohlschuetter
Copy link
Member

Thanks for your feedback @TW-Goldencode.

I think it becomes clear that 8MB is not a realistic upper limit, since 25MB datagrams seem to work for you as well.
I'm happy to see that the "max-capacity=0" approach works for you. However I think we can do better than that:

Please try the latest changes on that branch (including commit bf9fb50). That change lowers the limit back to 1MB, however it should work correctly (albeit perhaps a tad slower) for arbitrarily larger capacities.

Please let me know (after removing the max-capacity system property override from your VM config) how that new change performs compared to max-capacity=0.

Please (if possible) also test the scenario where you use direct byte buffers in the code that uses junixsocket (e.g., look for ByteBuffer.allocate and replace with ByteBuffer.allocateDirect).

@TW-Goldencode
Copy link
Author

@kohlschuetter I agree the new approach is far superior, thank you. The issue already is a ByteBuffer.allocateDirect scenario, does that surprise you? I use a NIO channel. The isDirect flow (from memory, on mobile now) was entered. I didn't analyze the complete code path.
I'll test on Monday and will get back to you.

@TW-Goldencode
Copy link
Author

@kohlschuetter Note: It's possible allocateDirect was only on the ServerSocket, and that's where the stream is fed, so write buffer space was plenty. The JVM also didn't cap the capacity under the hood (there's a JVM system variable for that, but the default suffices). More on Monday.

@TW-Goldencode
Copy link
Author

@kohlschuetter Tested bf9fb50 , it fixes the issue (with the 1MiB default limit, no system variables). Much preferred, performance is good.

Thanks again, we'll wait for release 2.5.2

kohlschuetter added a commit that referenced this issue Oct 4, 2022
@kohlschuetter
AFCore: Raise thread-local buffer capacity limit to 8 MB, add property
a068ed9 
We currently use ThreadLocal-defined direct byte buffers to allow
callers to use non-direct buffers where we really need direct ones.

The current maximum limit is 1MB, which breaks support for larger
datagrams.

Raise the limit from 1 MB to 8 MB, and allow configuration via a system
property, org.newsclub.net.unix.thread-local-buffer.max-capacity, which
takes the maximum capacity in bytes, or 0 for "unlimited".

Using 0 is highly discouraged, as it may effectively block large chunks
of memory.

#118
kohlschuetter added a commit that referenced this issue Oct 4, 2022
@kohlschuetter
AFCore: Revert limit to 1MB; allocate buffers on-the-fly if exceeded
34b5a5f 
The limit to how large datagram can be seems to have no feasibly low
limit (25MB datagrams have been reported to work). This imposes a
challenge on caching/reuse strategies for direct byte buffers (a shared,
reusable pool that is not thread-specific could be an alternative, but
comes at the cost of complexity).

At the cost of performance, revert the per-thread limit to 1MB, and
return newly allocated direct byte buffers instead of cached ones
whenever the limit is exceeded.

Users of such unexpectedly large datagrams could either still force a
higher (or unbounded) limit via the system property
"org.newsclub.net.unix.thread-local-buffer.max-capacity", or better,
use direct byte buffers in the calling code, obsoleting the need to
use this cache in the first place.

#118
@kohlschuetter kohlschuetter added bug The issue describes a bug in the code verify The issue is considered fixed/done, and reassigned to the originator to verify. labels Oct 5, 2022
@kohlschuetter kohlschuetter added this to the 2.5.2 milestone Oct 5, 2022
@kohlschuetter
Copy link
Member

junixsocket 2.5.2 has been released. Please re-open if you encounter further issues. Thanks again for reporting and testing, @TW-Goldencode!

@TW-Goldencode
Copy link
Author

Thank you @kohlschuetter , no issues yet. Upped our gradle to 2.5.2 . Great stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug The issue describes a bug in the code verify The issue is considered fixed/done, and reassigned to the originator to verify.
Projects
None yet
Milestone
2.5.2
Development

No branches or pull requests
2 participants
@kohlschuetter @TW-Goldencode