Avoids leaking Vert.x instances in the connection pool

[Sanne]

Fixes #129

Also makes it possible to inject a Vert.x instance whose lifecycle is managed externally.

[DavideD]

@Sanne, Can you rebase this to the latest changes please?

[gavinking]

Ouch, #156 conflicts nastily with #155 :-(

[DavideD]

Conflicts aside, LGTM

[gavinking]

So @Sanne how does this affect someone who wants to use HR inside Vert.x?

It looks like they'll have to write some additional code to inject the right instance of Vertx, but what does that look like?

(I assume that previously this was unnecessary, and HR just picked up the "default" instance of Vertx, right?.)

[gavinking]

@aguibert I suppose this will have some impact on the Quarkus extension, right?

[Sanne]

So @Sanne how does this affect someone who wants to use HR inside Vert.x?

Well they have two options:

1# don't do anything at all, it will still work just fine but we'll have a second Vert.x instance running within our internal services. Easy but slightly less efficient.

2# inject the Vert.x instance by using org.hibernate.reactive.service.ManagedVertxService and passing a vert.x instance at boot - in that case we'll switch to use the provided instance.

It looks like they'll have to write some additional code to inject the right instance of Vertx, but what does that look like?

StandardServiceRegistry registry = new StandardServiceRegistryBuilder()
.applySettings( properties )
.addService( VertxService.class, new ManagedVertxService( vertx ) );
.build();

In Quarkus we're more likely to rather inject the ReactiveConnectionProvider directly; but this is still useful for other cases.

(I assume that previously this was unnecessary, and HR just picked up the "default" instance of Vertx, right?.)

Well HR was invoking Vertx.vertx(), which actually builds and starts a new Vertx instance.. which noone was closing nor stopping.

[gavinking]

don't do anything at all, it will still work just fine but we'll have a second Vert.x instance running within our internal services. Easy but slightly less efficient.

OK I guess that doesn't sound too bad. I assumed that a single request shouldn't span multiple Vert.xes but if there's no problem with that I guess it's fine.

StandardServiceRegistry registry = new StandardServiceRegistryBuilder()
.applySettings( properties )
.addService( VertxService.class, new ManagedVertxService( vertx ) );
.build();

Well, not quite: if I'm not mistaken, they would have to also implement VertxService to wrap up their own instance of Vert.x, no?

In Quarkus we're more likely to rather inject the ReactiveConnectionProvider directly

Yes, that's why I tagged @aguibert on this. My guess is he will probably just replace the ReactiveConnectionPool.

Well HR was invoking Vertx.vertx(), which actually builds and starts a new Vertx instance.

Haha, oh really? I saw that code many times and just assumed it must be a singleton.

(It sure doesn't help that vertx() doesn't sound like a factory method, and that the method has zero Javadoc.)

[gavinking]

Well, not quite: if I'm not mistaken, they would have to also implement VertxService to wrap up their own instance of Vert.x, no?

Well, actually, it's a SMI. So no problem. We should annotate it @FunctionalInterface.

[Sanne]

Well, not quite: if I'm not mistaken, they would have to also implement VertxService to wrap up their own instance of Vert.x, no?

Well, actually, it's a SMI. So no problem. We should annotate it @FunctionalInterface.

No need, the ManagedVertxService is the wrapper service - it's included in HR

[Sanne]

Haha, oh really? I saw that code many times and just assumed it must be a singleton.

I assumed that too :) Noticed when capturing stacktraces to fine the leak.

So that's why I decided to extract the "reference to vertx" into a service. I'm quite sure we'll need it again..

[DavideD]

Haha, oh really? I saw that code many times and just assumed it must be a singleton.

I assumed the same :/

[gavinking]

No need, the ManagedVertxService is the wrapper service - it's included in HR

Well I was thinking of the following usecase:

StandardServiceRegistry registry = new StandardServiceRegistryBuilder()
    .applySettings( properties )
    .addService( VertxService.class, (c, r) -> myVertxThatImAlreadyUsing );
    .build();

People might not want to change their whole pre-existing app to use ManagedVertService.

[gavinking]

Oh, wait, I see what you're saying.

OK, so the name ManagedVertxService is a bit confusing. You mean "managed outside of HR".

But anyway, I think you can just get rid of it, can't you? VertxService is a SMI.

[Sanne]

No need, the ManagedVertxService is the wrapper service - it's included in HR

Well I was thinking of the following usecase:

StandardServiceRegistry registry = new StandardServiceRegistryBuilder()
    .applySettings( properties )
    .addService( VertxService.class, (c, r) -> myVertxThatImAlreadyUsing );
    .build();

People might not want to change their whole pre-existing app to use ManagedVertService.

Read my example again :) it's doing the same

But ok I'll make it FunctionalInterface as well, so people have options..

[gavinking]

Read my example again

Yes, I already did.

The naming is confusing.

[Sanne]

Remember we want to have the option to avoid lambdas in e.g. Quarkus

that just applies to our own code of course - end users are welcome to have fun with them.

[gavinking]

Remember we want to have the option to avoid lambdas in e.g. Quarkus

WDYM? The whole of HR is written using lambdas...

[gavinking]

@Sanne you're going to put this stuff somewhere under the pool package, right? I think that's probably where it belongs.

[Sanne]

ok I renamed the service and added javadoc. I'll move the packages next..

[gavinking]

Nope, you put it under service.

The thing is that this is going to be a user-visible API, and the service package is essentially implementation details. (And not even very elegant implementation at that.)

Also its only user is the SqlClientConnection stuff.

[Sanne]

Nope, you put it under service.

The thing is that this is going to be a user-visible API, and the service package is essentially implementation details. (And not even very elegant implementation at that.)

yes I hadn't received your messaget yet :) I said I renamed it, that was in response to "The naming is confusing."

[gavinking]

We're both commenting too fast :-)

[Sanne]

The thing is that this is going to be a user-visible API, and the service package is essentially implementation details. (And not even very elegant implementation at that.)

hum wait - this service stuff is indeed not elegant, and not meant as API in my plans: it's just a thing for "integrators" - say platforms and frameworks, appservers.

Sounds like you have the daily users in mind.. didn't we say we'll work on a nicely looking boostrap API for them in a second time?

[Sanne]

What about we merge it and then you move around as you prefer? I have no strong opinion regarding package, and am not really sure we're understanding each other.

I'd prefer to move on on the build issues assigned to me

[aguibert]

suppose this will have some impact on the Quarkus extension, right?

I don't think so. This only effects the path where HR builds a pool from individual config values (e.g. JDBC URL) and has to create the Vertx Pool on its own. In Quarkus we are already getting the Quarkus-created Pool passed into HR and set directly.

[gavinking]

@Sanne Well FTR, in my examples I use createEntityManagerFactory().unwrap(). That doesn't give them any good way to swap a service. (Except via META-INF/services and ServiceIntegrator I guess.)

But I think that's OK, as long as it works when we have a different vert.x to the one that is processing web requests.

Then for people who want to get fancy, StandardServiceRegistryBuilder isn't really so bad.

[Sanne]

ok I'll merge. @gavinking feel free to move the packages as you prefer ;)

[gavinking]

@aguibert In Quarkus we are already getting the Quarkus-created Pool passed into HR and set directly.

And how were you doing that? By replacing the ReactiveConnectionPool?

[gavinking]

@Sanne what about if we move it to a package called vertx?

[Sanne]

@Sanne what about if we move it to a package called vertx?

sure that sounds like a good one.

Regarding bootstrap: yes I agree that unwrap / StandardServiceRegistryBuilder aren't so bad - but on top of these people will need to also set some parameters, etc.. no need to get into details, but I just expect that we'll come with a better boot API which is specific for Reactive.

[gavinking]

I just expect that we'll come with a better boot API which is specific

Eventually, yes, I agree.

[aguibert]

And how were you doing that? By replacing the ReactiveConnectionPool?

@gavinking When a Vertx pool is explicitly provided in the configuration map it gets used here:

hibernate-reactive/hibernate-reactive-core/src/main/java/org/hibernate/reactive/pool/impl/SqlClientPool.java

Lines 63 to 69 in 1fcb8d3

	if (o != null) {
	if (!(o instanceof Pool)) {
	throw new ConfigurationException("Setting " + ReactiveSettings.VERTX_POOL + " must be configured with an instance of " +
	Pool.class.getCanonicalName() + " but was configured with " + o);
	} else {
	pool = (Pool) o;
	}

And if we find that we don't need to go down our own pool creation/configuration path.

That's how it worked when I originally wrote it at least. I may be missing a more subtle interaction of service/initiator changes that might now take precedence over that path though. From what I can tell that's not the case however.

[gavinking]

aaaaah, OK, I see, I had wondered several times what ReactiveSettings was for and where it came from. haha

OK so this now means we have two parallel ways to swap the instance of Vertx.

@aguibert do you think you could change the Quarkus extension to use the new VertxInstance service for this, i.e. overriding the service with your own impl?

[aguibert]

The Quarkus HR extension never references a Vertx instance directly, so I don't think that will be necessary. Rather, it just obtains a Vertx Pool from the appropriate Vertx-sql-client Quarkus extension and then sets that Pool instance on HR config (and the pool already contains a Quarkus-managed Vertx instance). When we go down the code path where a Pool instance is provide via config map (linked above), then serviceRegistry.getService( VertxService.class ); is never called.

[gavinking]

Ah, Ok, sorry, I misunderstood. You're passing in a Pool, not a Vertx.

Ok so then could you do it by moving that code to a subclass of SqlClientPool (just override start()) and installing that subclass as the ReactiveConnectionPool service?

WDYT?

[aguibert]

Yep that sounds reasonable. I'll re-sync my HR-Quarkus branch later today and report back if there are issues.

[emmanuelbernard]

Remember we want to have the option to avoid lambdas in e.g. Quarkus

WDYM? The whole of HR is written using lambdas...

Are you saying you create lambdas from withing Hibernate Reactive or rather that you receive lambdas from the user and go execute them?
What is costly with lambdas is the memory consumption (happens at creation time). That's why we removed them from most of our libraries. Users can sue them but we don't want to have undue memory consumed by our own internal usages.

[gavinking]

Are you saying you create lambdas from withing Hibernate Reactive or rather that you receive lambdas from the user and go execute them?

Both.

Writing reactive code without lambdas is extremely painful.

[gavinking]

I mean, within the codebase we have 296 lambdas. Changing them all to method calls would be pretty painful and make the code a lot uglier...

[aguibert]

Perhaps we could worry about de-lambda'ing the codebase after 1.0 when the code is settled down a bit more? And we could also evaluate the performance benefit and see if it even puts a dent in perf/memory

[Sanne]

Perhaps we could worry about de-lambda'ing the codebase after 1.0 when the code is settled down a bit more? And we could also evaluate the performance benefit and see if it even puts a dent in perf/memory

Sure. I personally try to avoid lambdas but I agree they help us move faster - and we need that now.

Just as a general rule, while writing new code if we can use a method reference rather than a lambda, I'd suggest preferring the method reference.

In my experience, capturing lambdas are a serious problem with allocation performance - and it's not always obvious for a lambda to be "capturing"; when thinking in terms of method references, any state that needs to be captured becomes more explicit.

[gavinking]

it's not always obvious for a lambda to be "capturing"

In dark mode, IntelliJ highlights all captured variables in a sickening shade of violet.

So it's pretty obvious.