Tapestry
  1. Tapestry
  2. TAPESTRY-2247

Transactions should roll back, not commit, at the end of each request

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 5.0.10
    • Fix Version/s: 5.0.12
    • Component/s: tapestry-hibernate
    • Labels:
      None

      Description

      On thread cleanup, the Session should be flushed not committed. If people want to commit changes they should explicitly do so but having the transaction committed automatically can lead to some very mysterious changing of the data. In my experience, having the transaction committed automatically does more harm than good.

        Activity

        Hide
        Massimo Lusetti added a comment -

        Well i probably don't get what you're referring to as "does more harm than good" but having to explicitly call commit() on plain successfull unit of operations seems a waste of time.

        Please would you like to share some more thoughts?

        Thanks

        Show
        Massimo Lusetti added a comment - Well i probably don't get what you're referring to as "does more harm than good" but having to explicitly call commit() on plain successfull unit of operations seems a waste of time. Please would you like to share some more thoughts? Thanks
        Hide
        Dan Adams added a comment -

        In T4 we had commits on by default for a while and it ended up causing some weirdness in our CMS projects; if you (or anything else) made a change to an entity then the changes could be persisted without you knowing. Let's say you had an add/edit page where the entity was the context. If you change some fields and then submit but some server-side validaton fails then the changes to the entity will be committed even though the user hasn't succesfully submit the form yet. So I would definitely want it to only save the changes once all the validation goes through. Another use case is if you have an add/edit page where the entity is persisted (normally) so that between screens it's not in the hibernate session. In this instance you actually have to do an explicity call to save() because the entity won't be attached to the session so it won't get caught in the automatic commit. Perhaps the behavior could be configurable with a symbol.

        Show
        Dan Adams added a comment - In T4 we had commits on by default for a while and it ended up causing some weirdness in our CMS projects; if you (or anything else) made a change to an entity then the changes could be persisted without you knowing. Let's say you had an add/edit page where the entity was the context. If you change some fields and then submit but some server-side validaton fails then the changes to the entity will be committed even though the user hasn't succesfully submit the form yet. So I would definitely want it to only save the changes once all the validation goes through. Another use case is if you have an add/edit page where the entity is persisted (normally) so that between screens it's not in the hibernate session. In this instance you actually have to do an explicity call to save() because the entity won't be attached to the session so it won't get caught in the automatic commit. Perhaps the behavior could be configurable with a symbol.
        Hide
        Massimo Lusetti added a comment -

        Well you certainly would to not submit (aka call session.save()) on entities till you're sure all validation and business check are in place so calling it before seems a very bad practice and should avoided.
        For the second use case, as you said if the entity is persisted into the session you have to reattach it to the hibernate session during subsequent utilization, so this would force me to call also a commit too (if needed to be committed). BTW persiting entities in the session is also another bad practice.

        I understand this as a way to encourage bad practices, they are probably widely used but still bad. Your propose of a configuration symbol is nice and i would probably make it even defaults to the current behaviour.

        Show
        Massimo Lusetti added a comment - Well you certainly would to not submit (aka call session.save()) on entities till you're sure all validation and business check are in place so calling it before seems a very bad practice and should avoided. For the second use case, as you said if the entity is persisted into the session you have to reattach it to the hibernate session during subsequent utilization, so this would force me to call also a commit too (if needed to be committed). BTW persiting entities in the session is also another bad practice. I understand this as a way to encourage bad practices, they are probably widely used but still bad. Your propose of a configuration symbol is nice and i would probably make it even defaults to the current behaviour.
        Hide
        Howard M. Lewis Ship added a comment -

        I this really an issue? In a typical case, the commit will occur on a context that has no outstanding issues.

        Ah, I see, the issue is with partial updates that are invalid. I think we need a larger strategy for handling this; a loose coupling between Form and the hibernate support so that the transaction can be rolled back, or marked rollback-only. Currently, the best option is to supply an onFailure() event handler to roll the transaction back.

        Show
        Howard M. Lewis Ship added a comment - I this really an issue? In a typical case, the commit will occur on a context that has no outstanding issues. Ah, I see, the issue is with partial updates that are invalid. I think we need a larger strategy for handling this; a loose coupling between Form and the hibernate support so that the transaction can be rolled back, or marked rollback-only. Currently, the best option is to supply an onFailure() event handler to roll the transaction back.
        Hide
        Howard M. Lewis Ship added a comment -

        Ultimately, we need to add some form of @Commit annotation as well, so that user code doesn't have to directly deal with the HibernateSessionManager.

        Show
        Howard M. Lewis Ship added a comment - Ultimately, we need to add some form of @Commit annotation as well, so that user code doesn't have to directly deal with the HibernateSessionManager.
        Hide
        Jonathan Barker added a comment -

        Perhaps the @Transactional annotation used by Spring would be a reasonable example to copy.

        Show
        Jonathan Barker added a comment - Perhaps the @Transactional annotation used by Spring would be a reasonable example to copy.
        Hide
        Howard M. Lewis Ship added a comment -

        Further evidence that automatically committing is a bug:

        Clients have experienced a race condition (under MySQL and Tomcat) where the autocommit was slower than the page refresh, resulting in a display of entities with state from before the commit. Using explicit commits ensures this won't happen, as a response won't be sent to the client until after the commit completes.

        Show
        Howard M. Lewis Ship added a comment - Further evidence that automatically committing is a bug: Clients have experienced a race condition (under MySQL and Tomcat) where the autocommit was slower than the page refresh, resulting in a display of entities with state from before the commit. Using explicit commits ensures this won't happen, as a response won't be sent to the client until after the commit completes.

          People

          • Assignee:
            Howard M. Lewis Ship
            Reporter:
            Dan Adams
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development