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

Changes from 1 commit
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