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.

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

Reenable TLS socket instrumentation by default #794

Open
schlosna opened this issue Jul 15, 2020 · 5 comments
Open

Reenable TLS socket instrumentation by default #794

schlosna opened this issue Jul 15, 2020 · 5 comments
Assignees

Comments

@schlosna
Copy link
Contributor

What happened?

OpenJDK's SSLSocket.addHandshakeCompletedListener(HandshakeCompletedListener) creates a HandshakeCompletedNotify-Thread for each TLS handshake, which under high throughput systems causes spikes in resource utilization and bottlenecks.

Added ability to disable TLS socket instrumentation to confirm in #790 and disabling the client side handshake instrumentation by default in #793

Pinged openjdk security-dev list to see if they're open to patches:

I'd like to revive this 7.5 year old thread for discussion as I have
seen seen several high throughput applications using
SSLSocket.addHandshakeCompletedListener(HandshakeCompletedListener)
where the "HandshakeCompletedNotify-Thread" created at
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/sun/security/ssl/TransportContext.java#L640
for each TLS handshake adds significant spike in resource utilization.
For example, a service receiving say 10,000 requests per second with
handshakes required for 0.5% we're creating 50 threads per second,
which is significant thrash. We would like to avoid this thread
creation to smooth out resource utilization.

I am tracking Project Loom's progress [1] where
HandshakeCompletedListener handling may be an ideal case for virtual
thread, but would like understand if folks here are open to patches to
improve this situation in the interim? Would utilizing a cached thread
pool be an acceptable patch request option? Would providing a system
property configurable option to execute the handlers directly on the
handshaking thread be an option (e.g. when a handler is lightweight
such as incrementing handshake, cipher suite, and protocol metrics)?

What did you want to happen?

Using HandshakeCompletedListener for instrumentation should not have bad performance side effects and TLS socket metrics should be enabled by default again.

@schlosna
Copy link
Contributor Author

Tagging @carterkozak for SA

@schlosna schlosna reopened this Jul 15, 2020
@carterkozak
Copy link
Contributor

woop, didn't meant o close this, my wires got crossed. I'll self-assign and track the conversation upstream.

@carterkozak carterkozak self-assigned this Jul 15, 2020
@schlosna
Copy link
Contributor Author

@carterkozak kicked off openjdk/loom#16 to use virtual thread for listener callback

@schlosna
Copy link
Contributor Author

OpenJDK tracking JDK-8246039

@carterkozak
Copy link
Contributor

The PR has merged into the loom project, we'll be able to re-enable our instrumentation (based on runtime detection) once loom is released.

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

No branches or pull requests

2 participants