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

improve Javadoc for VertxInstance stuff #1143

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

improve Javadoc for VertxInstance stuff #1143

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a15aad9
improve Javadoc for VertxInstance stuff
gavinking Jan 19, 2022
a7c4de1
improve documentation for VertxInstance stuff
gavinking Jan 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 19 additions & 4 deletions documentation/src/main/asciidoc/reference/introduction.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1053,10 +1053,25 @@ Hibernate Reactive session associated with the context.
=== Vert.x instance service

The `VertxInstance` service defines how Hibernate Reactive obtains an instance
of Vert.x. The default implementation just creates one the first time it's
needed. But if your program requires control over how the Vert.x instance is
created, or how it's obtained, you can override the default implementation and
provide your own `VertxInstance`. Let's consider this example:
of Vert.x when there is no instance associated with the calling thread. The
Copy link
Member

Choose a reason for hiding this comment

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

when there is no instance associated with the calling thread

Technically, it also defines how Hibernate Reactive obtains Vert.x even if there is one associated to the Thread.
Isn't this a bit misleading?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ummmmm. Hrrrrmmmm. Yeah, right. See there's a bit of a slightly confusing interplay between VertxContext and VertxInstance here. TBH I've never completely understood the significance of:

public void execute(Runnable runnable) {
    io.vertx.core.Context context = vertxInstance.getVertx().getOrCreateContext();
    if ( Vertx.currentContext() == context ) {
        runnable.run();
    }
    else {
        context.runOnContext( x -> runnable.run() );
    }
}

Like .... why don't we just use the Vertx.currentContext() if there is one? Why do we insist on it being the one coming from "our" Vert.x?

I mean, I know why we do it that way: because that's what @cescoffier and @vietj told us to do, but I think I need to understand it better.

Copy link
Member

Choose a reason for hiding this comment

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

  1. When I talk about "an instance of Vert.x", I mean io.vertx.core.Vertx.
  2. When I talk about "a Vert.x context", I mean io.vertx.core.Context.

So, DefaultVertxInstance is responsible for 1, org.hibernate.reactive.context.impl.VertxContext is responsible for 2.
But the VertxInstance service is always called when the factory is started (with or without an existing Vert.x instance), that's why I said that the line when there is no instance associated with the calling thread. doesn't seem correct (it implies that it gets called only when there is no Vert.x instance).

Copy link
Member

Choose a reason for hiding this comment

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

Like .... why don't we just use the Vertx.currentContext() if there is one? Why do we insist on it being the one coming from "our" Vert.x?

Doesn't it makes sure that 1. we are running in a context (it will create a new one if one doesn't exist) and 2. an existing session run the operations always in the same context and that should imply the same thread (this is the part where I'm never sure).

Copy link
Member

Choose a reason for hiding this comment

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

I think I need to think more about this

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I need to think more about this

Right, me too.

And I think perhaps we should wait until @Sanne gets back and discuss it thoroughly.

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't it makes sure that 1. we are running in a contex

I remember in the beginning we had a lot of issues with operations not running in the right context when using Quarkus.

default implementation just creates one the first time it's needed. But if your
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't it create one when the service is started? So when the SessionFactory is started

Copy link
Member Author

Choose a reason for hiding this comment

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

True. So then it's sort-of a bigger problem than I thought.

But default we go creating an instance of Vert.x even if we don't know we're going to need it.

program requires control over how the Vert.x instance is created, or how it's
obtained, you can override the default implementation and provide your own
`VertxInstance`.

Copy link
Member

@DavideD DavideD Jan 19, 2022 •
edited

Choose a reason for hiding this comment

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

I'm not sure about anything written after this point.

I think there are only two things to mention:

  • Hibernate Reactive will bind to the current Vertx instance if the factory is created inside an existing context (and will log a message about it)
  • Hibernate Reactive will create a new instance if none is detected when the factory is created

The user needs to do something only if this is not the behaviour they want.
There are many way to create a factory inside a context without using runOnContext.

Copy link
Member

@DavideD DavideD Jan 19, 2022 •
edited

Choose a reason for hiding this comment

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

Ah sorry, I mean it logs the message when a new Vertx instance is created

Copy link
Member

Choose a reason for hiding this comment

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

We should also mention that when Hibernate Reactive creates a new instance of Vert.x, it will also shut it down when the factory gets closed.

Copy link
Member Author

Choose a reason for hiding this comment

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

See what I documented is how I actually want it to work.

But you're right: it doesn't work like that today.

Copy link
Member Author

Choose a reason for hiding this comment

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

how I actually want it to work.

Well, naively.

IMPORTANT: If your program starts Vert.x externally to Hibernate Reactive, and
you don't tell Hibernate Reactive how to obtain this instance of Vert.x by
specifying an implementation of `VertxInstance`, you might end up with multiple
instances of `Vertx` in your program, resulting in confusing error messages.
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this a bug from our side? Shouldn't everything work anyway even if we start a different Vert.x instance?

Copy link
Member

Choose a reason for hiding this comment

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

Well, I would expect it to work if one uses .withSession.

Copy link
Member Author

Choose a reason for hiding this comment

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

If the program already has a Vertx, and we go and create one, then that's simply wrong, I would have thought.

Copy link
Member

@DavideD DavideD Jan 19, 2022 •
edited

Choose a reason for hiding this comment

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

I remember reading that one could use these scenario if they want different event busses for different groups of Verticles.
I might have dreamt it.
But, even if it's unlikely, I'm not sure it's wrong

Copy link
Member

Choose a reason for hiding this comment

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

Found it! It was in the Vert.x documentation:

Most applications will only need a single Vert.x instance, but it’s possible to create multiple Vert.x instances if you require, for example, isolation between the event bus or different groups of servers and clients.


There are two ways to solve this problem:

- make sure Hibernate Reactive is never called from a thread with no current
Vert.x context, by using `runOnContext()` in a disciplined way, as described
in the previous section, or
- provide your own implementation of `VertxInstance`.

Let's consider this example:

[source, JAVA, indent=0]
----
Expand Down
26 changes: 17 additions & 9 deletions hibernate-reactive-core/src/main/java/org/hibernate/reactive/vertx/VertxInstance.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,19 +7,27 @@

import io.vertx.core.Vertx;

import org.hibernate.reactive.pool.impl.DefaultSqlClientPool;
import org.hibernate.service.Service;

/**
* Used by {@link DefaultSqlClientPool} and
* {@link org.hibernate.reactive.context.impl.VertxContext}
* to obtain an instance of {@link Vertx}. The default instance is
* {@link org.hibernate.reactive.vertx.impl.DefaultVertxInstance}.
* Used by {@link org.hibernate.reactive.pool.impl.DefaultSqlClientPool}
* and {@link org.hibernate.reactive.context.impl.VertxContext} to
* obtain an instance of {@link Vertx}.
* <p>
* A program may integrate a custom {@link VertxInstance}
* with Hibernate Reactive by contributing a new service using a
* {@link org.hibernate.boot.registry.StandardServiceInitiator}
* or from code-based Hibernate configuration by calling
* The default implementation is
* {@link org.hibernate.reactive.vertx.impl.DefaultVertxInstance},
* which creates a new instance of {@code Vertx} if there is no
* instance already associated with the calling thread. This default
* behavior may cause problems in programs which have an instance of
* {@code Vertx} whose lifecycle is managed externally to Hibernate
* Reactive, and in such cases
* {@link org.hibernate.reactive.vertx.impl.ProvidedVertxInstance}
* or a custom-written {@code VertxInstance} should be used.
* <p>
* A program may integrate a custom {@link VertxInstance} with
* Hibernate Reactive by contributing a new service using a
* {@link org.hibernate.boot.registry.StandardServiceInitiator} or
* from code-based Hibernate configuration by calling the method
* {@link org.hibernate.reactive.provider.ReactiveServiceRegistryBuilder#addService}.
*
* <pre>{@code
Expand Down
8 changes: 7 additions & 1 deletion ...e-reactive-core/src/main/java/org/hibernate/reactive/vertx/impl/DefaultVertxInstance.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,15 @@
* A singleton instance of {@link Vertx} that is created on
* demand and destroyed automatically along with the Hibernate
* {@link org.hibernate.SessionFactory#close() session factory}.
* <p>
* Programs which require Hibernate reactive to use an instance
* of {@code Vertx} whose lifecycle is managed externally to
* Hibernate Reactive should use {@link ProvidedVertxInstance}
* instead.
*
* @author Sanne Grinovero <sanne@hibernate.org>
* @see ProvidedVertxInstance if you need to a different instance
*
* @see ProvidedVertxInstance
*/
public final class DefaultVertxInstance implements VertxInstance, Stoppable, Startable {

Expand Down
2 changes: 1 addition & 1 deletion ...-reactive-core/src/main/java/org/hibernate/reactive/vertx/impl/ProvidedVertxInstance.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@

/**
* An implementation of {@link VertxInstance} which allows the client
* to provide an instance of {@link Vertx} whose lifecyle is managed
* to provide an instance of {@link Vertx} whose lifecycle is managed
Copy link
Member

@DavideD DavideD Jan 19, 2022 •
edited

Choose a reason for hiding this comment

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

It's not part of your changes, but a few lines after this it states:

Hibernate will destroy the {@code Vertx} instance on shutdown.

This is not true though, isn't it?
a ProvidedVertxInstance won't do anything to the Vert.x instance when the factory is closed

Copy link
Member

@DavideD DavideD Jan 19, 2022 •
edited

Choose a reason for hiding this comment

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

I think it's copied from DefaultVertxInstance, since you are alredy updating this, could you fix this too, please?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yah, looks wrong.

* externally to Hibernate Reactive. The {@code ProvidedVertxInstance}
* must be registered explicitly with Hibernate by calling
* {@link org.hibernate.reactive.provider.ReactiveServiceRegistryBuilder#addService}.
Expand Down
4 changes: 3 additions & 1 deletion hibernate-reactive-core/src/main/java/org/hibernate/reactive/vertx/package-info.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
/**
* Integration with Vert.x.
* Integration with Vert.x. Allows a program to customize how
* instances of {@link io.vertx.core.Vertx} should be obtained
* by Hibernate Reactive.
*
* @see org.hibernate.reactive.vertx.VertxInstance
*/
Expand Down