Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: High High
    • Resolution: Fixed
    • Affects Version/s: 4.7 Maintenance, 5.3.12, 2017.12, 2018.06, 5.4.12, 2018.09
    • Fix Version/s: Customer request
    • Component/s: Legacy stack, QA
    • Labels:

      Description

      I have discovered a few issues related to how Legacy handles transactions during operations on versions (publishing, saving, discarding):

      1. EZP-29796 During the autosave, the status checks are performed (https://github.com/ezsystems/ezautosave/blob/master/classes/ezjscserverfunctionsautosave.php#L107) before the transaction begins (https://github.com/ezsystems/ezautosave/blob/master/classes/ezjscserverfunctionsautosave.php#L205). This means that if this version is published in the meantime (or archived, basically any status change) then during the transaction the status will be set back to draft, which leads to inconsistencies (no published version).

      2. EZP-29797 During discarding the draft, the attributes are deleted first and without the transaction (https://github.com/ezsystems/ezpublish-legacy-ee/blob/master/kernel/classes/ezcontentobjectversion.php#L923 and https://github.com/ezsystems/ezpublish-legacy-ee/blob/master/kernel/classes/ezcontentobjectversion.php#L927 ; `removeThis()` method call is not wrapped in the transaction either). If the request stops right before the transaction or the transaction rollbacks, then attributes will be deleted, but version with the draft will still be there, which in turn makes the draft no longer editable (if you try to edit this version, then no attributes are shown, so there is nothing to edit).

      3. EZP-29798 During discarding the draft, the status checks are performed (https://github.com/ezsystems/ezpublish-legacy-ee/blob/master/kernel/content/removeeditversion.php#L32) before the transaction begins. This means that if this version is published in the meantime (or archived, basically any status change) then during the transaction it will be removed, which leads to inconsistencies (no published version).

      4. EZP-29799 During the publishing of the draft, the status of the current version is changed to Archived. However, the current version is retrieved from the object which is cached in a global variable (https://github.com/ezsystems/ezpublish-legacy-ee/blob/master/kernel/content/ezcontentoperationcollection.php#L134), so the current version might be outdated if another version is published simultaneously. If this happens, the Content Object will have two Published versions.

      5. EZP-29800 (with DFS configured) During the cache purge, called by example via php bin/php/ezcache.php --clear-all --purge command, the _purgeByLike method is called (https://github.com/ezsystems/ezpublish-legacy-ee/blob/5.4/kernel/private/classes/clusterfilehandlers/dfsbackends/mysqli.php#L337). This method deletes file metadata in the transaction, however, it also deletes the physical file regardless of the transaction result: https://github.com/ezsystems/ezpublish-legacy-ee/blob/5.4/kernel/private/classes/clusterfilehandlers/dfsbackends/mysqli.php#L393. If the transaction fails, this causes the content view cache to NOT be regenerated correctly (because the metadata for the cache file still exists) and instead the blank page is displayed.

        Issue Links

          Activity

          Hide
          Gunnstein Lye added a comment -

          Expanding transactions to wrap all preliminary queries is easy. It remains to be seen if there is any nasty performance impact. As I understand Mysql, it will do non-blocking reads where possible. If we hit cases where it blocks the content object or attribute tables for any amount of time, concurrent editing grinds to a halt. Any solution needs extensive testing.
          https://dev.mysql.com/doc/refman/5.7/en/innodb-consistent-read.html

          Show
          Gunnstein Lye added a comment - Expanding transactions to wrap all preliminary queries is easy. It remains to be seen if there is any nasty performance impact. As I understand Mysql, it will do non-blocking reads where possible. If we hit cases where it blocks the content object or attribute tables for any amount of time, concurrent editing grinds to a halt. Any solution needs extensive testing. https://dev.mysql.com/doc/refman/5.7/en/innodb-consistent-read.html
          Hide
          Gunnstein Lye added a comment - - edited

          Early work in progress
          Point 1: https://github.com/ezsystems/ezautosave/pull/27
          Points 2-5: https://github.com/ezsystems/ezpublish-legacy/pull/1380 (this one is closed, using separate PRs for each newly created sub-task instead)

          Show
          Gunnstein Lye added a comment - - edited Early work in progress Point 1: https://github.com/ezsystems/ezautosave/pull/27 Points 2-5: https://github.com/ezsystems/ezpublish-legacy/pull/1380 (this one is closed, using separate PRs for each newly created sub-task instead)
          Hide
          Gunnstein Lye added a comment - - edited

          It's clear by now that this won't be easy. Expanding transactions is not enough by itself, and can have negative side effects. The solution may require explicit locks, and DB brand dependent code. I have split this into its 5 parts, to work on them as separate issues.

          For the last issue (autosave) to be complete, it seems to require changes to the datatype base class(es) and special implementation for some types, so an API change is needed. This is not done and may not get done, it may be too big a change for legacy.

          For testing purposes I have collected all current PRs into one each for kernel and autosave repo. So these two together contains the current state of fixes as of 2019-01-04.
          https://github.com/ezsystems/ezpublish-legacy/pull/1415
          https://github.com/ezsystems/ezautosave/pull/27

          Show
          Gunnstein Lye added a comment - - edited It's clear by now that this won't be easy. Expanding transactions is not enough by itself, and can have negative side effects. The solution may require explicit locks, and DB brand dependent code. I have split this into its 5 parts, to work on them as separate issues. For the last issue (autosave) to be complete, it seems to require changes to the datatype base class(es) and special implementation for some types, so an API change is needed. This is not done and may not get done, it may be too big a change for legacy. For testing purposes I have collected all current PRs into one each for kernel and autosave repo. So these two together contains the current state of fixes as of 2019-01-04. https://github.com/ezsystems/ezpublish-legacy/pull/1415 https://github.com/ezsystems/ezautosave/pull/27
          Hide
          Gunnstein Lye added a comment - - edited

          QA, please sanity check these two PRs together:
          https://github.com/ezsystems/ezpublish-legacy/pull/1415 merged in https://github.com/ezsystems/ezpublish-legacy/commit/8e58bc6db28518c384334dfc407b557a308c1fec
          https://github.com/ezsystems/ezautosave/pull/27 merged in https://github.com/ezsystems/ezautosave/commit/733aa9e239c063b9c7209ebafda1004e12321904
          If you have setups for high load concurrency testing, please do that too.

          Aspects for sanities: Discard draft, publish draft, autosave, DFS cache purge.

          Show
          Gunnstein Lye added a comment - - edited QA, please sanity check these two PRs together: https://github.com/ezsystems/ezpublish-legacy/pull/1415 merged in https://github.com/ezsystems/ezpublish-legacy/commit/8e58bc6db28518c384334dfc407b557a308c1fec https://github.com/ezsystems/ezautosave/pull/27 merged in https://github.com/ezsystems/ezautosave/commit/733aa9e239c063b9c7209ebafda1004e12321904 If you have setups for high load concurrency testing, please do that too. Aspects for sanities: Discard draft, publish draft, autosave, DFS cache purge.
          Hide
          Michał Szołtysek added a comment -

          Retested successfully on v5.3.12.1 with diffs from both above PRs.

          Show
          Michał Szołtysek added a comment - Retested successfully on v5.3.12.1 with diffs from both above PRs.
          Hide
          Tyler Harms added a comment -

          Are there plans to push this to the 5.4.x branch?

          Show
          Tyler Harms added a comment - Are there plans to push this to the 5.4.x branch?
          Hide
          Gunnstein Lye added a comment -

          Tyler Harms Yes. ETA unknown due to high load on QA, but I expect to make a 5.4 release that includes this.

          Show
          Gunnstein Lye added a comment - Tyler Harms Yes. ETA unknown due to high load on QA, but I expect to make a 5.4 release that includes this.

            People

            • Assignee:
              Unassigned
              Reporter:
              Sylvain Guittard
            • Votes:
              3 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Time Tracking

                Estimated:
                Original Estimate - 0 minutes
                0m
                Remaining:
                Remaining Estimate - 0 minutes
                0m
                Logged:
                Time Spent - 2 weeks, 1 day, 5 hours, 47 minutes
                2w 1d 5h 47m