Use Jdk heap buffer pooling in Reactive SQL Clients

[tsegismont]

See #42224

To avoid having several sys props for config, reuse the existing one related to http server.

[tsegismont]

cc @franz1981

[quarkus-bot]

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 445f09f.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

[github-actions]

🎊 PR Preview d702f49 has been successfully built and deployed to https://quarkus-pr-main-46865-preview.surge.sh/version/main/guides/

• Images of blog posts older than 3 months are not available.
• Newsletters older than 3 months are not available.

[cescoffier]

I can't remember if the system property was documented (I think we said we wanted to keep it hidden for now). If it's documented we should amend the documentation to mention this new usage.

[tsegismont]

It's purposely not documented (for expert users only):

#42224 (comment)

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 445f09f.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

---

Flaky tests - Develocity

⚙️ JVM Tests - JDK 17 Windows

📦 extensions/hibernate-orm/deployment

✖ io.quarkus.hibernate.orm.applicationfieldaccess.PublicFieldAccessInheritanceTest.testFieldAccess - History

• Expecting actual not to be null - java.lang.AssertionError

java.lang.AssertionError: 

Expecting actual not to be null
	at io.quarkus.hibernate.orm.applicationfieldaccess.PublicFieldAccessInheritanceTest$FieldAccessEnhancedDelegate$1.assertValue(PublicFieldAccessInheritanceTest.java:141)
	at io.quarkus.hibernate.orm.applicationfieldaccess.PublicFieldAccessInheritanceTest.doTestFieldAccess(PublicFieldAccessInheritanceTest.java:100)
	at io.quarkus.hibernate.orm.applicationfieldaccess.PublicFieldAccessInheritanceTest.testFieldAccess(PublicFieldAccessInheritanceTest.java:61)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)
	at io.quarkus.test.QuarkusUnitTest.runExtensionMethod(QuarkusUnitTest.java:521)

📦 extensions/micrometer-opentelemetry/deployment

✖ io.quarkus.micrometer.opentelemetry.deployment.compatibility.MicrometerTimedInterceptorTest.testTimeMethod_Failed - History

• Stream has no elements - java.lang.IllegalArgumentException

java.lang.IllegalArgumentException: Stream has no elements
	at io.quarkus.micrometer.opentelemetry.deployment.common.MetricDataFilter.lastReadingDataPoint(MetricDataFilter.java:236)
	at io.quarkus.micrometer.opentelemetry.deployment.compatibility.MicrometerTimedInterceptorTest.testTimeMethod_Failed(MicrometerTimedInterceptorTest.java:100)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)
	at io.quarkus.test.QuarkusUnitTest.runExtensionMethod(QuarkusUnitTest.java:521)
	at io.quarkus.test.QuarkusUnitTest.interceptTestMethod(QuarkusUnitTest.java:435)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

[franz1981]

Hi @tsegismont similarly to the test performed by other users, please verify that it works as expected. In theory the difference is so big which should be a net win 🙏

[tsegismont]

Hi @tsegismont similarly to the test performed by other users, please verify that it works as expected. In theory the difference is so big which should be a net win 🙏

I've created this project for testing: https://github.com/tsegismont/quarkus-ssl-jdk-buffer-pooling

So far, this PR has no effect.

With some debugging, I noticed that there is a difference between client/server when vert.x chooses the allocator.
https://github.com/franz1981/vert.x/blob/a3393ace29680911e11cb8b6705c2369964dbf98/src/main/java/io/vertx/core/net/impl/SSLHelper.java

For servers:

  ByteBufAllocator serverByteBufAllocator(SslContextProvider ctxProvider) {
    if (!ssl || usesJDKSSLWithPooledHeapBuffers(ctxProvider)) {
      return PooledByteBufAllocator.DEFAULT;
    }
    return PartialPooledByteBufAllocator.INSTANCE;
  }

For clients:

  ByteBufAllocator clientByteBufAllocator(SslContextProvider ctxProvider) {
    if (usesJDKSSLWithPooledHeapBuffers(ctxProvider)) {
      return PooledByteBufAllocator.DEFAULT;
    }
    return PartialPooledByteBufAllocator.INSTANCE;
  }

  private boolean usesJDKSSLWithPooledHeapBuffers(SslContextProvider ctxProvider) {
    return ssl && sslEngineOptions instanceof JdkSSLEngineOptions &&
      ctxProvider.jdkSSLProvider() &&
      ((JdkSSLEngineOptions) sslEngineOptions).isPooledHeapBuffers();
  }

With PgClient, this ssl flag is never set to true, because it's not used. The connection is upgraded if needed:

https://github.com/eclipse-vertx/vertx-sql-client/blob/abee64760761e491df5e06c77b34ba53f7461c4e/vertx-pg-client/src/main/java/io/vertx/pgclient/impl/PgConnectionFactory.java#L143

@franz1981 why is there a difference between the two methods in the PR you sent? eclipse-vertx/vert.x#5316

[franz1981]

why is there a difference between the two methods in the PR you sent?

It was to grant that without the configured option (i.e isPooledHeapBuffers) it was correctly working as it was before and the server side was already using PooledByteBufAllocator with ssl == false as per https://github.com/eclipse-vertx/vert.x/pull/5316/files#diff-88382d98b04bb14d16dc4c52c85bc92a6edd504510cfaceec2255cf1de1db256L245-L249 - while the client side, nope.

With PgClient, this ssl flag is never set to true, because it's not used. The connection is upgraded if needed:

Argh! Actually I've implemented what I've done assuming HTTP (which doesn't have a common Upgrade happening - most of the time, at least).
It means that vertx side should be "fixed" to handle this situation, because I understand that if the connection is already established and need to be upgraded to use SSL, the decision about which allocator to use has already been performed.
Is it like that?

Another option would be to rework what I've done in Vertx and just have a sys property which enable using the right allocator regardless - overriding the decision upfront: I've discarded this option at the time of the change, to save having a global behaviour enforced - but it's likely the easiest solution.

[tsegismont]

I understand that if the connection is already established and need to be upgraded to use SSL, the decision about which allocator to use has already been performed.
Is it like that?

Exactly

[tsegismont]

@franz1981 More details about upstream

The ability to switch between the Vert.x so-called partial byte buffer allocator and the default pooled allocator was introduced in 4.2 with eclipse-vertx/vert.x#4110

A key point is that buffers are converted to "safe" buffers if they were allocated from any pooling allocator (or they are composite).

To my knowledge, this is safety check is in place for both servers and clients so, with some extra tests to make sure all is good, I think we could merge the two methods in SSLHelper into one:

  ByteBufAllocator byteBufAllocator(SslContextProvider ctxProvider) {
    if (!ssl || usesJDKSSLWithPooledHeapBuffers(ctxProvider)) {
      return PooledByteBufAllocator.DEFAULT;
    }
    return PartialPooledByteBufAllocator.INSTANCE;
  }

Another option would be to rework what I've done in Vertx and just have a sys property which enable using the right allocator regardless - overriding the decision upfront: I've discarded this option at the time of the change, to save having a global behaviour enforced - but it's likely the easiest solution.

That is certainly cheaper to do, and given the sysprop is for advanced users and not documented, perhaps the way to go.

But before getting started, I'd like to know if @vietj has any comment on this.

[franz1981]

Just to add some color why it could be concerning to use a partial allocator as the one defined in vertx: https://github.com/eclipse-vertx/vert.x/blob/adbe97600cc6215f15ce2fac629c89f93618ca8f/src/main/java/io/vertx/core/buffer/impl/PartialPooledByteBufAllocator.java#L44

This is overriding the buffer method which is not just available to users and internal vertx code, but to Netty itself, see https://github.com/netty/netty/blob/0ec94939a1deb86b4bd96e6c3198304acad8e1ae/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectEncoder.java#L328

Which shows that Netty can start to unexpectedly allocate big unpooled heap buffers where it usually allocate direct pooled ones.