Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.6
    • Fix Version/s: 1.6.6, 1.7.4, 1.8.4, 1.9
    • Component/s: Calendar
    • Labels:
      None
    • Environment:
      All
    • Affected Branches:
      MOODLE_16_STABLE
    • Fixed Branches:
      MOODLE_16_STABLE, MOODLE_17_STABLE, MOODLE_18_STABLE, MOODLE_19_STABLE
    • Rank:
      28115

      Description

      I've just realised there's quite a serious race condition for how repeat events are handled in the calendar. I thought the repeatid column just pointed to the id of the original event.

      But, no:

      if ($form->repeat) {
      $fetch = get_record_sql('SELECT 1, MAX(repeatid) AS repeatid FROM '.$CFG->prefix.'event');
      $form->repeatid = empty($fetch) ? 1 : $fetch->repeatid + 1;
      }

      cvs:/calendar/edit.php

        Activity

        Hide
        Martin Dougiamas added a comment -

        From (penny at catalyst.net.nz) Thursday, 29 June 2006, 07:12 AM:

        adding martinl to cc list

        From Luke Hudson (lingo at paradise.net.nz) Friday, 28 July 2006, 05:36 AM:

        The attached patch solves this problem using the following method:

        When a discussion is moved to a new forum, a skeleton discussion is left behind with the same title, but with (Moved) appended to it. The discussion will contain only one post, with the text:

        This discussion has been moved to here, in the forum (new location)

        The user can click either the 'here' or the name of the new forum to go the the relevant places.

        From Luke Hudson (lingo at paradise.net.nz) Friday, 28 July 2006, 05:37 AM:

        Oh, bother. I've put this in the wrong place

        Please excuse me, this attachemnt and message refer to MDL-4874

        Show
        Martin Dougiamas added a comment - From (penny at catalyst.net.nz) Thursday, 29 June 2006, 07:12 AM: adding martinl to cc list From Luke Hudson (lingo at paradise.net.nz) Friday, 28 July 2006, 05:36 AM: The attached patch solves this problem using the following method: When a discussion is moved to a new forum, a skeleton discussion is left behind with the same title, but with (Moved) appended to it. The discussion will contain only one post, with the text: This discussion has been moved to here, in the forum (new location) The user can click either the 'here' or the name of the new forum to go the the relevant places. From Luke Hudson (lingo at paradise.net.nz) Friday, 28 July 2006, 05:37 AM: Oh, bother. I've put this in the wrong place Please excuse me, this attachemnt and message refer to MDL-4874
        Hide
        Martín Langhoff added a comment -

        Luke has a patch that I like, and agree with. The bit that I am not 100% familiar with is the change to $backup_version in backup/version.php – I am not familiar with how that is managed.

        http://git.catalyst.net.nz/gitweb?p=moodle-r2.git;a=commitdiff;h=5e256e8a1f283883c18efdef669b26bdaa8eba45;hp=8f12f5e1b93b27c1479548ce6b0106408445944a

        The patch has my signoff except for that detail. If Eloy or someone else knowledgeable in backup versioning nods...

        Show
        Martín Langhoff added a comment - Luke has a patch that I like, and agree with. The bit that I am not 100% familiar with is the change to $backup_version in backup/version.php – I am not familiar with how that is managed. http://git.catalyst.net.nz/gitweb?p=moodle-r2.git;a=commitdiff;h=5e256e8a1f283883c18efdef669b26bdaa8eba45;hp=8f12f5e1b93b27c1479548ce6b0106408445944a The patch has my signoff except for that detail. If Eloy or someone else knowledgeable in backup versioning nods...
        Hide
        Martín Langhoff added a comment -

        Added Eloy (who I'd like to review the patch) and Luke (who's coded it in the first place!)

        Show
        Martín Langhoff added a comment - Added Eloy (who I'd like to review the patch) and Luke (who's coded it in the first place!)
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Well, patch looks really outdated now (old upgrade scripts and so on). Some questions to understand better what's wrong:

        1) When a calendar event is saved with n repeats, n records are created in the event table all them sharing the repeatid value (pointing to the first event of the group). That should be the correct behaviour, yep?

        2) The race condition happens when inserting new event "groups" yep?

        If both answers are yes, then I think that:

        1) The "calendar/event.php" patch looks perfect, because it enforces 1) perfectly.
        2) The "backup/restorelib.php" patch looks glorious too, because enforces 1) perfectly.
        3) It's not needed to increment the backup version at all.
        4) About upgrade scripts... I really would leave them unmodified. It's a "rare" race condition and iterating over all repeated events can be heavy in some servers. In fact, I think that current queries in the patch are useless, because if the race condition has arrived, then two events will share the "repeatid" and we cannot differentiate them at all by "repeatid" (an that seems to be the base for those upgrade queries).

        Any comment?

        Show
        Eloy Lafuente (stronk7) added a comment - Well, patch looks really outdated now (old upgrade scripts and so on). Some questions to understand better what's wrong: 1) When a calendar event is saved with n repeats, n records are created in the event table all them sharing the repeatid value (pointing to the first event of the group). That should be the correct behaviour, yep? 2) The race condition happens when inserting new event "groups" yep? If both answers are yes, then I think that: 1) The "calendar/event.php" patch looks perfect, because it enforces 1) perfectly. 2) The "backup/restorelib.php" patch looks glorious too, because enforces 1) perfectly. 3) It's not needed to increment the backup version at all. 4) About upgrade scripts... I really would leave them unmodified. It's a "rare" race condition and iterating over all repeated events can be heavy in some servers. In fact, I think that current queries in the patch are useless, because if the race condition has arrived, then two events will share the "repeatid" and we cannot differentiate them at all by "repeatid" (an that seems to be the base for those upgrade queries). Any comment?
        Hide
        Martín Langhoff added a comment -

        Good point (4) that if the race condition has hit, the data is probably already messed up, and not fixable. +1 for applying the core of the patch.

        Show
        Martín Langhoff added a comment - Good point (4) that if the race condition has hit, the data is probably already messed up, and not fixable. +1 for applying the core of the patch.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Done, from 16_STABLE to HEAD. Fixed.

        Thanks all! Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Done, from 16_STABLE to HEAD. Fixed. Thanks all! Ciao

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: