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

Thread-based nesting behavior for managed handles is surprising and not adequately documented #2633

Open
jpallas opened this issue Feb 14, 2024 · 5 comments
Assignees
Labels
answered doc waiting-for-op Waiting for a response from the original poster

Comments

@jpallas
Copy link

jpallas commented Feb 14, 2024

It's possible I've misunderstood or overlooked something, but as best I can tell:

The documentation on managed transactions says

The inTransaction and useTransaction methods on the Jdbi object create a new Handle, open a transaction and pass it to the callback code.

The inTransaction and useTransaction methods on Handle objects open a transaction on the handle itself and then pass it to the callback code. By default, if these methods are called while the handle is already in an open transaction, the existing transaction is reused (no nested transaction is created).

That says nothing about the behavior of non-transactional handle methods, and it doesn't mention reusing handles by the managed handle methods on Jdbi.

The section on obtaining a managed handle does not mention anything about nesting behavior. Reading it gives no reason to think that these methods might not return a new handle, and more importantly, might not return an auto-commit handle if the calling thread is inside the callback for a transactional handle.

But, in the implementation, it appears that this reuse of managed handles actually applies to both transactional handles and non-transactional handles. I had code that indirectly called Jdbi.useHandle nested within a call to Jdbi.inTransaction, and the nested call did not get an autocommit handle, which caused much confusion and led to a deadlock situation.

See #1992 (comment) for the only mention I could find that this automatic reuse of a Handle within a thread applies to the non-transactional methods Jdbi.useHandle and Jdbi.withHandle.

At the very least, this needs better documentation. And it would be nice if there were a way to explicitly indicate that I don't want to reuse a handle in a nested situation. The workaround I used was a try-with-resources on Jdbi.open with a call to Handle.inTransaction. This seems to skip the Thread-local handle machinery.

Also, the documentation could be clearer that nesting is based on Thread identity, so might not work with certain async programming models.

@hgschmie
Copy link
Contributor

Hi @jpallas,

Thank you for the detailed explanation. The docs do state very clearly (in https://jdbi.org/#_handle) that Handle and all its attached objects are intended for use by a single thread and not thread-safe.

In general, we strive for the code to fail in obvious ways and report errors clearly. I would love to learn more about the patterns that you are using that led to deadlock situations.

A handle is ultimately a stand-in for a JDBC connection. Most database driver connection objects are not threadsafe and many don't even support sharing a connection object across multiple threads. There is nothing that Jdbi can do about it.

I will look through your comments and clarify the documentation if necessary. If you have a test case where you can reproduce the failures / deadlock situations, we can also improve the code behavior.

@hgschmie
Copy link
Contributor

hgschmie commented Feb 16, 2024

reading through your comments, I am trying to understand this sentence:

That says nothing about the behavior of non-transactional handle methods, and it doesn't mention reusing handles by the managed handle methods on Jdbi.

What method are you referring to? It is correct, that the Handle object has a number of methods that start a transaction on the handle, but what is a managed handle method on Jdbi? Are you doing something like this:

jdbi.useHandle(handle -> {
      ...
      jdbi.useHandle(xxx -> {
         ...
      });
});

In that case, handle and xxx point at the same object. This is a documentation issue and I will add a comment to the managed handle section.

hgschmie added a commit to hgschmie/jdbi that referenced this issue Feb 16, 2024
@jpallas pointed out in jdbi#2633 that our documentation about nesting
callbacks with managed handles is incomplete and confusing.

Add a chapter about nested callback, call out the limitations, add
references to the managed handle and managed transaction chapters.
@hgschmie hgschmie self-assigned this Feb 16, 2024
@hgschmie hgschmie added doc answered waiting-for-op Waiting for a response from the original poster labels Feb 16, 2024
@stevenschlansker
Copy link
Member

Thanks for the feedback @jpallas . I appreciate the frustration when things do not work the way you expect, but the reentrant behavior of Handles and Transactions is not new and provides a lot of benefit in the common case of writing synchronous code.

You mention asynchronous programming. JDBC and therefore Jdbi is not asynchronous by their nature. There are some attempts to build this (R2DBC?) but personally we find that since you have a very limited connection pool anyway, it is no problem to also have a thread pool for the sole purpose of dispatching db ops and completing results.

You can see some documentation on that here: https://jdbi.org/#_usage_in_asynchronous_applications

@hgschmie
Copy link
Contributor

I also added explicit documentation about the behavior in nested calls in #2635.

hgschmie added a commit to hgschmie/jdbi that referenced this issue Feb 18, 2024
@jpallas pointed out in jdbi#2633 that our documentation about nesting
callbacks with managed handles is incomplete and confusing.

Add a chapter about nested callback, call out the limitations, add
references to the managed handle and managed transaction chapters.
@jpallas
Copy link
Author

jpallas commented Feb 18, 2024

reading through your comments, I am trying to understand this sentence:

That says nothing about the behavior of non-transactional handle methods, and it doesn't mention reusing handles by the managed handle methods on Jdbi.

What method are you referring to? It is correct, that the Handle object has a number of methods that start a transaction on the handle, but what is a managed handle method on Jdbi? Are you doing something like this:

jdbi.useHandle(handle -> {
      ...
      jdbi.useHandle(xxx -> {
         ...
      });
});

In that case, handle and xxx point at the same object. This is a documentation issue and I will add a comment to the managed handle section.

Yes, the methods I'm referring to are the ones on the Jdbi object, not the handle, and what my code was doing was like the example above, with some key differences:

jdbi.useTransaction(handle -> {
    // do some operations with the handle that must not commit yet
    ...
    // indirect call through an object that performs bookkeeping
    // we don't see in the source code that there is nesting happening (it is not lexically nested, it is dynamically nested)
    jdbi.useHandle(handle -> {
        // perform bookkeeping, expecting this operation to autocommit
        // updates to a bookkeeping table cause locks to be taken, which would be released by the commit
    }
    // having returned from indirect call, start doing work while intentionally holding the transaction open
    // spawn worker threads that try to do bookkeeping about their progress (using new handles, because they are different threads)
    // worker threads are blocked because of locks still being held on the bookkeeping table
    ...
    // wait for worker threads to finish
    // now transaction can commit (or rollback if workers reported failures)
}

I've tried to explain in the comments above why I'm doing something that may sound a little odd.

The key thing is that the Javadoc didn't warn that jdbi.useHandle might not return a handle in autocommit mode. At first face, that's the fundamental difference in intent between useHandle and useTransaction, so it never occurred to me that calling useTransaction in one place might change the behavior of useHandle somewhere else ("spooky action at a distance").

I hope that sheds light on how I got surprised.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
answered doc waiting-for-op Waiting for a response from the original poster
Development

No branches or pull requests

3 participants