java.jdbc

Requesting a nested transaction with a more restrictive isolation level appears to succeed, but does not have the requested effect

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None
  • Patch:
    Code and Test

Description

The library helpfully allows nested transactions by maintaining an internal depth counter, but silently disregards any transaction options for nested transactions. This has actually bitten me recently in production; code that required a serializable transaction in order to maintain an invariant was inadvertently being executed within a repeatable-read transaction.

This patch changes the behavior to raise if a nested transaction requests a different isolation level in which the actual transaction is running.

A similar problem exists for nested transactions that request read-only mode. I will be happy to modify the patch to cover that case if you are interested in accepting this.

Activity

Hide
Sean Corfield added a comment -

Thanks Donald. I'll take a look at this.

You don't appear to be listed here: http://clojure.org/contributing – have you signed and submitted the Contributor's Agreement?

Show
Sean Corfield added a comment - Thanks Donald. I'll take a look at this. You don't appear to be listed here: http://clojure.org/contributing – have you signed and submitted the Contributor's Agreement?
Hide
Donald Ball added a comment -

Not yet, but I will do so forthwith.

Show
Donald Ball added a comment - Not yet, but I will do so forthwith.
Hide
Donald Ball added a comment -

Okay, I'm legal.

Show
Donald Ball added a comment - Okay, I'm legal.
Hide
Sean Corfield added a comment -

Great! I'll try to take a look at this over the weekend.

Show
Sean Corfield added a comment - Great! I'll try to take a look at this over the weekend.
Hide
Sean Corfield added a comment -

Thanks for the patch Donald!

Show
Sean Corfield added a comment - Thanks for the patch Donald!
Hide
Donald Ball added a comment -

Would you be amenable to also raising if the inner transaction requests an incompatible read-only mode?

Show
Donald Ball added a comment - Would you be amenable to also raising if the inner transaction requests an incompatible read-only mode?
Hide
Sean Corfield added a comment -

Sure. Do you want to create a new ticket and attach a patch to it?

Show
Sean Corfield added a comment - Sure. Do you want to create a new ticket and attach a patch to it?
Hide
Sean Corfield added a comment -

Donald Ball This issue came up again recently (as a request to allow the old behavior), and I wondered if you ever created the issue for the incompatible read-only mode check? I don't see it...

Show
Sean Corfield added a comment - Donald Ball This issue came up again recently (as a request to allow the old behavior), and I wondered if you ever created the issue for the incompatible read-only mode check? I don't see it...
Hide
Donald Ball added a comment -

I never did get around to it, no sorry! Would you like it, or are you reconsidering the change in general? (I still feel the new behavior is more correct, but am curious if there's a use case I failed to consider.)

Show
Donald Ball added a comment - I never did get around to it, no sorry! Would you like it, or are you reconsidering the change in general? (I still feel the new behavior is more correct, but am curious if there's a use case I failed to consider.)
Hide
Sean Corfield added a comment -

I'd be happy for you to create an issue for that (and offer a patch if you get the time).

To partially address the request from the (java.jdbc) mailing list, I added a get-isolation-level function so folks can at least tell if they're inside a transaction and what that current isolation level is (if known).

Show
Sean Corfield added a comment - I'd be happy for you to create an issue for that (and offer a patch if you get the time). To partially address the request from the (java.jdbc) mailing list, I added a get-isolation-level function so folks can at least tell if they're inside a transaction and what that current isolation level is (if known).

People

Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: