supercargo / guice-persist-jooq

Guice-persist extension for using jOOQ based persistence layer
Apache License 2.0
22 stars 10 forks source link

Default configuration uses just one connection #11

Closed MichaelBlume closed 3 years ago

MichaelBlume commented 4 years ago

We initially used guice-persist-jooq using the code sample from the README, leaving out the "optional" method:

    protected void configure() {
        install(new JooqPersistModule());
        bind(DataSource.class).to(ManagedDataSource.class);
    }

    @Provides ManagedDataSource dataSource(final ManagedDataSourceFactory factory) throws ClassNotFoundException {
        return factory.build(configuration);
    }

    @Provides
    public SQLDialect dialect() {
        //TODO read from DB configuration
        return SQLDialect.POSTGRES;
    }       

This resulted in an injected DSLContext object which already held a reference to a single connection -- when we restart our database, this connection closes, and the app becomes unusable until we restart it. The workaround was to include the "optional" method from the readme:

    public Configuration configuration(DataSource dataSource, SQLDialect dialect) {
            return new DefaultConfiguration()
                    .set(dataSource)
                    .set(dialect);
    }

It seems like in the absence of a method providing Configuration, this should be the default behavior? jooq should get a datasource, rather than a single connection.

supercargo commented 4 years ago

In the absence of the optional jooq Configuration, the default behavior is to retrieve a new connection from the DataSource at the beginning of each unit-of-work and to close that connection at the end of the unit-of-work. Of course, typically, you'd use a pooling DataSource implementation to manage the underlying connections.

My expectation would be that in case of a DB server restart any in-progress units of work would fail (and, presumably, transactionally roll back) but that once the database became available again future units of work would behave correctly. Based on the issue you're having, it sounds like you're holding on to DSLContext acquired in one unit-of-work and then continuing to use it in subsequent units-of-work. And since ending a unit-of-work closes the connection held by DSLContext, it also seems like maybe you are starting but never ending these units-of-work?

Can you clarify the lifecycle of the DSLContext you're getting injected? It might be worth adding some documentation on how this is expected to map into the Guice Persist execution model.

MichaelBlume commented 4 years ago

Good question, the DAO I was having trouble with looks like so

public class JooqAuthTokenDao implements AuthTokenDao {

  @Inject
  protected DSLContext database;

  ...
}

Which is again I think the pattern recommended in the README?

And then our application is started and managed using dropwizard-guicey

supercargo commented 4 years ago

I’m not familiar with dropwizard-guicey, but in JAX-RS in general, resource classes tend to be instantiated once per request unless you declare them as Singleton or do something else to manage their lifecycle. Guice behaves the same way by default...everything is instantiated as needed from the injector unless you scope your injectables differently.

I’m happy to include more examples or clarify the documentation if you’re seeing different behavior with dropwizard-guicey. Ultimately there may not be any one right answer, and as you noticed you can take control of Connection lifecycle by using your own configuration. It just seems in conflict with the approach guice-persist takes around unit-of-work.

supercargo commented 4 years ago

@MichaelBlume Did you ever figure out where your lifecycle issues are stemming from? I'd like to at least include a note in the example if this is something that would be wrong-by-default for dropwizard-guicey, especially since I included a Dropwizard example in the README