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

improve Javadoc for VertxInstance stuff #1143

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

Conversation

gavinking
Copy link
Member

No description provided.

@@ -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

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

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.

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.

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
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

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

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

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.

@gavinking gavinking marked this pull request as draft April 11, 2023 22:01
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.

2 participants