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

Enable buffer pooling settings with a SSL configurable option #5316

Open
wants to merge 1 commit into
base: 4.x
Choose a base branch
from

Conversation

franz1981
Copy link
Contributor

SSL JDK can really be a memory hell, see netty/netty#14208: using unpooled buffers means trouble - but still there are users which "maybe" want to keep things as they are.

I've created an "hidden" property (which should become the default in vertx 5 really...) to have forcibly pooling of heap buffers using JDK SSL, but, by default it will keep the behaviour as it is.

@franz1981
Copy link
Contributor Author

@cescoffier and @vietj PTAL

I've added some asserts to make it existing tests to fail and make sure the invariant I'm assuming for SSL on vertx are correct.

@franz1981
Copy link
Contributor Author

@cescoffier this is key for both https and vertx sql client - cause the latter will rely heavily on it for the reactive sql drivers (both mysql and postgres AFAIK) -

@tsegismont AFAIK vertx sql driver should rely on NetClientImpl e.g. https://github.com/eclipse-vertx/vertx-sql-client/blob/master/vertx-sql-client/src/main/java/io/vertx/sqlclient/impl/ConnectionFactoryBase.java#L45
hence I would expect that the changes on NetClientImpl's allocator should just do "the right thing", wdyt?

@franz1981 franz1981 marked this pull request as draft September 17, 2024 08:50
@franz1981
Copy link
Contributor Author

After talking with @cescoffier I'm changing this back to use a specific option to enable this feature.

@franz1981 franz1981 force-pushed the 4.x_unified_allocator_fix_ssl branch from 2dc914b to 0fca2ca Compare October 1, 2024 13:24
@franz1981 franz1981 changed the title Adding sys prop to enable heap pools with JDK SSL Enable buffer pooling settings with a SSL configurable option Oct 1, 2024
@franz1981 franz1981 marked this pull request as ready for review October 1, 2024 13:25
@franz1981
Copy link
Contributor Author

franz1981 commented Oct 1, 2024

PTAL @vietj

I've added some test (I could improve thsi further) - but it is not clear to me yet the role of https://vertx.io/docs/apidocs/io/vertx/core/http/HttpClientOptions.html#setSsl-boolean-

And indeed it seems that ssl, whilst false can still build a JDK EngineConfig, see

sslContextFactorySupplier = Future.succeededFuture(new EngineConfig(SslProvider.JDK, sslOptions, () -> new DefaultSslContextFactory(SslProvider.JDK, false), SSLEngineOptions.DEFAULT_USE_WORKER_POOL));

can you help me to understand what's ssl is meant to configure and how to use it?

import io.vertx.test.tls.Cert;
import io.vertx.test.tls.Trust;

public class SSLAllocatorTest extends VertxTestBase {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cescoffier not yet perfect but decent enough to make sure the default behaviour is still the same
I need to wait till this is merged to add some CI run which enable both this and the unified allocator changes at #5292 , which is ready for being reviewed too (by @vietj )

@franz1981
Copy link
Contributor Author

@tsegismont
I need you here bud: how do I benefit from this change into vert-x sql client?
How we can consume it for vert-x sql client too into Quarkus?

i.e. Here I need users to set https://vertx.io/docs/apidocs/io/vertx/core/net/TCPSSLOptions.html#setSsl-boolean- and https://vertx.io/docs/apidocs/io/vertx/core/net/TCPSSLOptions.html#setSslEngineOptions-io.vertx.core.net.SSLEngineOptions- (passing a JDK SSL option which enable heap buffer pooling), in order to made vertx to do the "right" thing and avoid quarkusio/quarkus#41880 (comment) to happen.

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.

1 participant