Moodle
  1. Moodle
  2. MDL-23273

limit of responses at module choice fail at synchronous submissions

    Details

    • Testing Instructions:
      Hide

      Setup:

      • course with atleast 2 students (or admin enrolled in course and a student)
      • make a 'choice' with the following settings changed.
        • Options:
          • Limit the number of responses allowed -> Yes
          • Option 1: anything here, Limit 1
          • Option #: anything here, Limit 1 (make as many options as you want)
        • Results:
          • Publish results -> Always show results to students
          • Privacy of results -> Publish full results, showing names and their choices

      Test One:

      • Go to the Choice as Student 1 and select Option 1 DONT HIT "SAVE MY CHOICE"
      • Open an incognito tab (or new browser) and login as Student 2 and selection Option 1 as well. Again, not saving it.
      • Go into mod/choice/lib.php and place sleep(10); after the lock is used 272.
      • Submit student ones choice.
      • Remove the sleep.
      • Submit student two's choice.

      Expected The first student to save the result successfully. The second student will display an notification error "The choice is full and there are no available places"

      Test Two:

      • Same setup as above, make sure the answers for the choices are cleared for students one and two.
      • Go into mod/choice/lib.php and place sleep(10); after the the insert, but before the lock is released. (Line 391 is good)
      • Submit student ones choice.
      • Remove the sleep.
      • Submit student two's choice.

      Expected The first student to save the result successfully. The second student will display an notification error "The choice is full and there are no available places"

      Test Three:

      • In adding the locks I've moved the placing of the event recording. Ensure the Course logs report the correct events have occurred and been logged.
      • Where? Course Admin ► Reports / ► Logs.
      • Example:

        18 Feb, 07:47	Admin User	-	Choice: TESTING	Choice	Choice made	The user with id '2' made the choice with id '1' in the choice activity with course module id '6'.	web	172.XXX.XXX.XXX

      Other testing:

      • Relevant unit tests:

        vendor/bin/phpunit mod/choice/tests/events_test.php

      • Play around with submitting random options and deleting them to make sure nothing has been broken.
      • Are there cases where the lock isn't released properly? (exceptions etc, specifically the print_error calls)
      • Timeouts reached under a reasonable load?
      Show
      Setup: course with atleast 2 students (or admin enrolled in course and a student) make a 'choice' with the following settings changed. Options: Limit the number of responses allowed -> Yes Option 1: anything here, Limit 1 Option #: anything here, Limit 1 (make as many options as you want) Results: Publish results -> Always show results to students Privacy of results -> Publish full results, showing names and their choices Test One: Go to the Choice as Student 1 and select Option 1 DONT HIT "SAVE MY CHOICE" Open an incognito tab (or new browser) and login as Student 2 and selection Option 1 as well. Again, not saving it. Go into mod/choice/lib.php and place sleep(10); after the lock is used 272. Submit student ones choice. Remove the sleep. Submit student two's choice. Expected The first student to save the result successfully. The second student will display an notification error "The choice is full and there are no available places" Test Two: Same setup as above, make sure the answers for the choices are cleared for students one and two. Go into mod/choice/lib.php and place sleep(10); after the the insert, but before the lock is released. (Line 391 is good) Submit student ones choice. Remove the sleep. Submit student two's choice. Expected The first student to save the result successfully. The second student will display an notification error "The choice is full and there are no available places" Test Three: In adding the locks I've moved the placing of the event recording. Ensure the Course logs report the correct events have occurred and been logged. Where? Course Admin ► Reports / ► Logs. Example: 18 Feb, 07:47 Admin User - Choice: TESTING Choice Choice made The user with id '2' made the choice with id '1' in the choice activity with course module id '6'. web 172.XXX.XXX.XXX Other testing: Relevant unit tests: vendor/bin/phpunit mod/choice/tests/events_test.php Play around with submitting random options and deleting them to make sure nothing has been broken. Are there cases where the lock isn't released properly? (exceptions etc, specifically the print_error calls) Timeouts reached under a reasonable load?
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_27_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_27_STABLE, MOODLE_28_STABLE
    • Pull from Repository:
    • Pull 2.7 Branch:
    • Pull 2.8 Branch:
    • Pull Master Branch:
      MDL-23273-master
    • Sprint:
      Team Beards Sprint 3
    • Issue size:
      Small

      Description

      To reproduce the bug:
      1.create a new course or use an existing one
      2.add item 'choice' with defaults values excpet the following settings:

      Limit the number of responses allowed = enable
      some choices where limit = 1
      Restrict answering to this time period = yes
      Display vertically
      Publish results = always show results to students
      privacy of results = publish full results, showing names and their choices
      group mode = separate groups

      3.login at 2 different PCs with different student account
      4.select same choice and hit 'save my choice' synchronously

      As you can see in the attachment, choosing a limited choice is possible. We have received some complaints by teachers using this tool. They report that (somehow) this effect happens quite often, especially when tight time period is enabled.

      Thank you for your help in advance!!!

        Gliffy Diagrams

        1. choice_limit_responseMoodle2_6.diff
          8 kB
          Claus A. Us.
        1. invalid choice.png
          137 kB
        2. screenshot_choice.jpg
          85 kB
        3. screenshot_settings.jpg
          153 kB
        4. valid choice.png
          99 kB

          Issue Links

            Activity

            Hide
            Dan Marsden added a comment -

            Unfortunately there isn't much we can do about this for 1.9Stable, but we can use the transaction support in 2.0 to improve this a bit (only for databases that support transactions) - It's important to note that MySQL MyIsam tables don't support transactions.

            I'll look at adding transaction support to to 2.01 (unless I get to it earlier.)

            Show
            Dan Marsden added a comment - Unfortunately there isn't much we can do about this for 1.9Stable, but we can use the transaction support in 2.0 to improve this a bit (only for databases that support transactions) - It's important to note that MySQL MyIsam tables don't support transactions. I'll look at adding transaction support to to 2.01 (unless I get to it earlier.)
            Hide
            Gergely Rakoczi added a comment -

            Dear Dan,
            thank you for your helpful information! As a consequence we're waiting for Moodle 2.01.

            Show
            Gergely Rakoczi added a comment - Dear Dan, thank you for your helpful information! As a consequence we're waiting for Moodle 2.01.
            Hide
            Andrea Bicciolo added a comment -

            Hi Dan,

            it appears to me that a similar issue happens also when Limit=1 for most (or all choice) even without synchronous submissions.

            Logs report two student able to select the seam choice (limited at 1) in two different days. In this case transaction support should not be necessary. Maybe it could worth taking a look: Dan I can provide cases if you want to further explore.

            Show
            Andrea Bicciolo added a comment - Hi Dan, it appears to me that a similar issue happens also when Limit=1 for most (or all choice) even without synchronous submissions. Logs report two student able to select the seam choice (limited at 1) in two different days. In this case transaction support should not be necessary. Maybe it could worth taking a look: Dan I can provide cases if you want to further explore.
            Hide
            David Campbell added a comment -

            I posted this also in the choice module forum.

            I just used the choice activity to have students preregister for 7 courses. There were a variety of limits for the different sections, but in some cases the number of students exceeded the set limt. For example, I set a 20 person limit for the Monday 2nd period section, but 29 were able to select that section. This only happened to 3 other sections and they were ones with higher limits. I don't know if this had anything to do with it, but the server was hit pretty hard. When the activity opened there about 300 users trying to make selections and the server really slowed down. A couple of students that I know of also got database errors. None of the students unenroled during that time, but they were able update their choices.

            Show
            David Campbell added a comment - I posted this also in the choice module forum. I just used the choice activity to have students preregister for 7 courses. There were a variety of limits for the different sections, but in some cases the number of students exceeded the set limt. For example, I set a 20 person limit for the Monday 2nd period section, but 29 were able to select that section. This only happened to 3 other sections and they were ones with higher limits. I don't know if this had anything to do with it, but the server was hit pretty hard. When the activity opened there about 300 users trying to make selections and the server really slowed down. A couple of students that I know of also got database errors. None of the students unenroled during that time, but they were able update their choices.
            Hide
            Claus A. Us. added a comment -

            First of all, this problem occurs in version 2.5.2 as well.
            We had courses with 500 students who tried to use choice module as first come first serve distribution of courses. The choices were timed, thus most of the students visited our page at the same time. (We do not recommend this module for large courses because of the heavy server load generated when students try to access the module, but we cannot prevent teachers from using it in these situations.)
            We recognized a further unwanted behaviour: some students even were able to choose an option twice or even three times. If this happens, the limit of that option can never be reached (under usual conditions), because the option is closed since the user is counted twice. Nevertheless the overview does not list the user twice, that list shows a distinct list. Thus, if the limit is 30 the list shows 29 users and on the same time no other user can choose that option as it is marked as closed.
            I suppose using transaction leads to problems with exceeding the connection pool, and thus lead to database connection errors. Maybe a solution would be a double check, a second check, that checks if limit is exceeded and the choice is revoked.

            Show
            Claus A. Us. added a comment - First of all, this problem occurs in version 2.5.2 as well. We had courses with 500 students who tried to use choice module as first come first serve distribution of courses. The choices were timed, thus most of the students visited our page at the same time. (We do not recommend this module for large courses because of the heavy server load generated when students try to access the module, but we cannot prevent teachers from using it in these situations.) We recognized a further unwanted behaviour: some students even were able to choose an option twice or even three times. If this happens, the limit of that option can never be reached (under usual conditions), because the option is closed since the user is counted twice. Nevertheless the overview does not list the user twice, that list shows a distinct list. Thus, if the limit is 30 the list shows 29 users and on the same time no other user can choose that option as it is marked as closed. I suppose using transaction leads to problems with exceeding the connection pool, and thus lead to database connection errors. Maybe a solution would be a double check, a second check, that checks if limit is exceeded and the choice is revoked.
            Hide
            jose antonio rodriguez added a comment -

            Hi,
            We have noticed that when we set up the choice with the option NO GROUPS, then it works correctly.

            Show
            jose antonio rodriguez added a comment - Hi, We have noticed that when we set up the choice with the option NO GROUPS, then it works correctly.
            Hide
            Monash University VLE team added a comment -

            Occurring in 2.3.2 and 2.5.5 at the moment.

            Show
            Monash University VLE team added a comment - Occurring in 2.3.2 and 2.5.5 at the moment.
            Hide
            Claus A. Us. added a comment -

            Dear Jose,
            It should not be a difference whether you enable group mode or not. Your tests were not good enough. Under high load conditions there will still be an overbooking.
            Hi all,
            I added a diff with changes I made to overcome the problem. The fix solves the issue of overbooking by double-checking if the limit is reached after the choice is saved to the database. In case of overbooking the choice is deleted again. This is not best practice coding since using transactions would be the way of choice, but this approach seems reasonable. This fix is in use since March. The limits did not exceeded till then although we had some choices causing high load.
            Nevertheless with that fix there is a limitation or maybe a bug fix for another problem This diff file changes the behaviour of the module a bit:
            Usually the choice module did not include choices of unenrolled students for calculating the number of submitted choices. It was possible that someone has chosen an option and afterwards the limit was reached. This person can then unenrol form course and another student is able to choose that option. After enrolling again, the choice of the former unenrolled student is considered again for the calculation the limit and thus the limit is exceeded. The result now lists unenrolled choices as well, but instead of printing the username an “unenrolled” entry is added. It can be deleted by the teacher.

            Show
            Claus A. Us. added a comment - Dear Jose, It should not be a difference whether you enable group mode or not. Your tests were not good enough. Under high load conditions there will still be an overbooking. Hi all, I added a diff with changes I made to overcome the problem. The fix solves the issue of overbooking by double-checking if the limit is reached after the choice is saved to the database. In case of overbooking the choice is deleted again. This is not best practice coding since using transactions would be the way of choice, but this approach seems reasonable. This fix is in use since March. The limits did not exceeded till then although we had some choices causing high load. Nevertheless with that fix there is a limitation or maybe a bug fix for another problem This diff file changes the behaviour of the module a bit: Usually the choice module did not include choices of unenrolled students for calculating the number of submitted choices. It was possible that someone has chosen an option and afterwards the limit was reached. This person can then unenrol form course and another student is able to choose that option. After enrolling again, the choice of the former unenrolled student is considered again for the calculation the limit and thus the limit is exceeded. The result now lists unenrolled choices as well, but instead of printing the username an “unenrolled” entry is added. It can be deleted by the teacher.
            Hide
            Claus A. Us. added a comment -

            Moodle 2.6:
            Changes:

            • prevents overbooking by double checking limit after inserting choice to DB
            • considers unenrolled students for limit
            • enables deletion of unenrolled choices by teacher
            Show
            Claus A. Us. added a comment - Moodle 2.6: Changes: prevents overbooking by double checking limit after inserting choice to DB considers unenrolled students for limit enables deletion of unenrolled choices by teacher
            Hide
            Dan Marsden added a comment -

            Thanks Claus - looks like some interesting code there.

            No existing Modules show enrolled and un-enrolled user data to teachers that I know of but we have to deal with the issue where course data is kept for a user when un-enrolled and this causes problems for the choice limit if they re-enrol again. I still haven't worked out the best way to handle un-enrolled user data but I'm open to ideas (and patches!)

            The double checking of the limit is something that I'm sure the code used to do (pre Moodle 2) but someone refactored that at some point and it was lost

            Show
            Dan Marsden added a comment - Thanks Claus - looks like some interesting code there. No existing Modules show enrolled and un-enrolled user data to teachers that I know of but we have to deal with the issue where course data is kept for a user when un-enrolled and this causes problems for the choice limit if they re-enrol again. I still haven't worked out the best way to handle un-enrolled user data but I'm open to ideas (and patches!) The double checking of the limit is something that I'm sure the code used to do (pre Moodle 2) but someone refactored that at some point and it was lost
            Hide
            Dan Marsden added a comment -

            unassigning myself from this issue so that someone else can pick it up if they want to but I'll still keep an eye on comments/patches etc

            Show
            Dan Marsden added a comment - unassigning myself from this issue so that someone else can pick it up if they want to but I'll still keep an eye on comments/patches etc
            Hide
            Paul Holden added a comment -

            MDL-12083 deals with removing user choice submissions when the user is unenrolled from a course; this would prevent one of the causes of choice limits being exceeded

            Show
            Paul Holden added a comment - MDL-12083 deals with removing user choice submissions when the user is unenrolled from a course; this would prevent one of the causes of choice limits being exceeded
            Hide
            Zachary Durber added a comment -

            Thank you for the hard work and investigating this, I've taken the code and written up some commits for it.

            I personally am not a fan of the error message but it's in keeping with how similar messages are handled for mod_choice.

            I've not included the enrolment/ un-enrolled counting code as we are just tackling the failing synchronous submissions at this point.

            Show
            Zachary Durber added a comment - Thank you for the hard work and investigating this, I've taken the code and written up some commits for it. I personally am not a fan of the error message but it's in keeping with how similar messages are handled for mod_choice. I've not included the enrolment/ un-enrolled counting code as we are just tackling the failing synchronous submissions at this point.
            Hide
            CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-23273 using repository: git://github.com/zbdd/moodle.git

            More information about this report

            Show
            CiBoT added a comment - Fails against automated checks. Checked MDL-23273 using repository: git://github.com/zbdd/moodle.git MOODLE_27_STABLE (9 errors / 0 warnings) [branch: MDL-23273-27 | CI Job ] phplint (0/0) , php (8/0) , js (0/0) , css (0/0) , phpdoc (1/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , MOODLE_28_STABLE (2 errors / 0 warnings) [branch: MDL-23273-28 | CI Job ] phplint (0/0) , php (1/0) , js (0/0) , css (0/0) , phpdoc (1/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , master (2 errors / 0 warnings) [branch: MDL-23273-master | CI Job ] phplint (0/0) , php (1/0) , js (0/0) , css (0/0) , phpdoc (1/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
            Hide
            Zachary Durber added a comment -

            benchmarking showed little to no change with pageload and db read/writes over 2 tests using 2 browsers and 3 sessions with and without the changes.

            Show
            Zachary Durber added a comment - benchmarking showed little to no change with pageload and db read/writes over 2 tests using 2 browsers and 3 sessions with and without the changes.
            Hide
            CiBoT added a comment -

            Code verified against automated checks.

            Checked MDL-23273 using repository: git://github.com/zbdd/moodle.git

            More information about this report

            Show
            CiBoT added a comment - Code verified against automated checks. Checked MDL-23273 using repository: git://github.com/zbdd/moodle.git MOODLE_27_STABLE (0 errors / 0 warnings) [branch: MDL-23273-27 | CI Job ] MOODLE_28_STABLE (0 errors / 0 warnings) [branch: MDL-23273-28 | CI Job ] master (0 errors / 0 warnings) [branch: MDL-23273-master | CI Job ] More information about this report
            Hide
            Dan Marsden added a comment -

            Hi Zachary, thanks for starting on this but I don't think that code is complete enough to land in core... from what I can see it changes the behaviour of the Choice to include un-enrolled users in the limits. The existing code doesn't do this.

            Show
            Dan Marsden added a comment - Hi Zachary, thanks for starting on this but I don't think that code is complete enough to land in core... from what I can see it changes the behaviour of the Choice to include un-enrolled users in the limits. The existing code doesn't do this.
            Hide
            Zachary Durber added a comment -

            Thanks Dan, I've amended the commits to check for enrolled users.

            Couple points.
            I've noticed on 2.7 users who have made a choice, then have gone on to have their enrolment removed still count towards a choice. This is separate to my code.
            On 2.8 stable and master my changes now ignore no longer enrolled users whom have made a choice previously.

            In testing I've found that if a user re-enrols after previously making a choice they can bypass the limit that way. This is separate to this issue and looks like it's relevant to MDL-12083.

            Show
            Zachary Durber added a comment - Thanks Dan, I've amended the commits to check for enrolled users. Couple points. I've noticed on 2.7 users who have made a choice, then have gone on to have their enrolment removed still count towards a choice. This is separate to my code. On 2.8 stable and master my changes now ignore no longer enrolled users whom have made a choice previously. In testing I've found that if a user re-enrols after previously making a choice they can bypass the limit that way. This is separate to this issue and looks like it's relevant to MDL-12083 .
            Hide
            Dan Marsden added a comment -

            I'm pretty sure you should be doing something with groups in there too....

            re-enrol and bypassing the limit is the "expected behaviour" which should be dealt with in MDL-12083 - feel free to un-assign me there and propose a "master only" improvement to deal with that better!

            Show
            Dan Marsden added a comment - I'm pretty sure you should be doing something with groups in there too.... re-enrol and bypassing the limit is the "expected behaviour" which should be dealt with in MDL-12083 - feel free to un-assign me there and propose a "master only" improvement to deal with that better!
            Hide
            Zachary Durber added a comment -

            No worries Dan and thanks for staying with me on this issue. I'm going through and procedurally improving the code and testing for edge cases as I do. Just found a few bugs in writing this that distracted me from pushing my latest changes.

            Show
            Zachary Durber added a comment - No worries Dan and thanks for staying with me on this issue. I'm going through and procedurally improving the code and testing for edge cases as I do. Just found a few bugs in writing this that distracted me from pushing my latest changes.
            Hide
            Zachary Durber added a comment - - edited

            Okay, so after discussing with Damyon Wiese I've dropped the original attempt to fix this in favour of cleaning up the code and using transactions as it's the only surefire way of ensuring we don't keep hitting this problem.

            Additionally, I've altered the existing code so that it sends back a status message to output as a notification. The error message previously if the choice was full, was an error page and you would be redirected to the course-main page which felt counter-intuitive.
            see the attached screenshots for comparisons

            The way the limit was being checked (now within a transaction) was also potentially confusing so I cleaned up choiceexceeded to say maxchoicesreached and altered the flow of a conditional to be more straightforward.

            Edit: For 2.7 I only applied the synchronous submission bug fix not the visual changes. The code differs from 2.7 - 2.8+ so I kept it largely the same.

            Show
            Zachary Durber added a comment - - edited Okay, so after discussing with Damyon Wiese I've dropped the original attempt to fix this in favour of cleaning up the code and using transactions as it's the only surefire way of ensuring we don't keep hitting this problem. Additionally, I've altered the existing code so that it sends back a status message to output as a notification. The error message previously if the choice was full, was an error page and you would be redirected to the course-main page which felt counter-intuitive. see the attached screenshots for comparisons The way the limit was being checked (now within a transaction) was also potentially confusing so I cleaned up choiceexceeded to say maxchoicesreached and altered the flow of a conditional to be more straightforward. Edit: For 2.7 I only applied the synchronous submission bug fix not the visual changes. The code differs from 2.7 - 2.8+ so I kept it largely the same.
            Hide
            CiBoT added a comment -

            Code verified against automated checks.

            Checked MDL-23273 using repository: git://github.com/zbdd/moodle.git

            More information about this report

            Show
            CiBoT added a comment - Code verified against automated checks. Checked MDL-23273 using repository: git://github.com/zbdd/moodle.git MOODLE_27_STABLE (0 errors / 0 warnings) [branch: MDL-23273-27 | CI Job ] MOODLE_28_STABLE (0 errors / 0 warnings) [branch: MDL-23273-28 | CI Job ] master (0 errors / 0 warnings) [branch: MDL-23273-master | CI Job ] More information about this report
            Hide
            Zachary Durber added a comment -

            The behaviour Andrea Bicciolo commented about earlier, where asynchronous submissions over different days were still able to by pass the limit is resolved too. (In the patch line 317 of mod/choice/lib.php)

            Show
            Zachary Durber added a comment - The behaviour Andrea Bicciolo commented about earlier, where asynchronous submissions over different days were still able to by pass the limit is resolved too. (In the patch line 317 of mod/choice/lib.php)
            Hide
            Ankit Agarwal added a comment -

            Hi Zac,
            Thanks for working on this issue. A few comments:-

            1. Are you fully sure option id is always unique for a given user for various choice types ? Else this will fail - https://github.com/zbdd/moodle/compare/eb1dc9f...MDL-23273-master#diff-c3eb8f93fb42c0b7a1210ebf0f093b5fR204
            2. use isset instead of array_key_exists(), much faster.
            3. Function description should come before params - https://github.com/zbdd/moodle/compare/eb1dc9f...MDL-23273-master#diff-c3eb8f93fb42c0b7a1210ebf0f093b5fR249
            4. \choice_user_submit_response() has almost no comments, if you are updating the api and it's behavior I would suggest making it a bit more clear what the various if blocks are for.
            5. Please separate your changes into multiple commits. One should contain the transaction related stuff and other should have the improvements you are doing. The improvements should only go to master unless absolutely necessary.
            6. Missing tags - ui_change, and possibly docs_required and release_notes
            7. Behavior of the api is changed, this needs an upgrade.txt mention.
            8. I personally don't like the return behavior of the \choice_user_submit_response() in the new code. Imo, it should always return a string. Anyway you don't have to change it, if you prefer it this way.
            9. You have a transaction but you are not doing a rollback incase of errors. This could potentially hold the lock until the db connection is closed.
            10. It seems to be that a lock suits our needs more than a transaction. However I have not considered this in detail, just something for you to consider.
            11. I don't think we should be triggering events inside a transaction, since event handlers can be talking to external systems about actions which eventually could be rolled back. Events should always be triggered after a successfull transaction.

            Cheers

            Show
            Ankit Agarwal added a comment - Hi Zac, Thanks for working on this issue. A few comments:- Are you fully sure option id is always unique for a given user for various choice types ? Else this will fail - https://github.com/zbdd/moodle/compare/eb1dc9f...MDL-23273-master#diff-c3eb8f93fb42c0b7a1210ebf0f093b5fR204 use isset instead of array_key_exists(), much faster. Function description should come before params - https://github.com/zbdd/moodle/compare/eb1dc9f...MDL-23273-master#diff-c3eb8f93fb42c0b7a1210ebf0f093b5fR249 \choice_user_submit_response() has almost no comments, if you are updating the api and it's behavior I would suggest making it a bit more clear what the various if blocks are for. Please separate your changes into multiple commits. One should contain the transaction related stuff and other should have the improvements you are doing. The improvements should only go to master unless absolutely necessary. Missing tags - ui_change, and possibly docs_required and release_notes Behavior of the api is changed, this needs an upgrade.txt mention. I personally don't like the return behavior of the \choice_user_submit_response() in the new code. Imo, it should always return a string. Anyway you don't have to change it, if you prefer it this way. You have a transaction but you are not doing a rollback incase of errors. This could potentially hold the lock until the db connection is closed. It seems to be that a lock suits our needs more than a transaction. However I have not considered this in detail, just something for you to consider. I don't think we should be triggering events inside a transaction, since event handlers can be talking to external systems about actions which eventually could be rolled back. Events should always be triggered after a successfull transaction. Cheers
            Hide
            Zachary Durber added a comment -

            Thanks Ankit that was very thorough. Just commenting to say that I'm implementing the changes now.

            Show
            Zachary Durber added a comment - Thanks Ankit that was very thorough. Just commenting to say that I'm implementing the changes now.
            Hide
            Zachary Durber added a comment -
            • I've scrapped 1 & 2.
            • Corrected placement of function description.
            • Commented choice_user_submit_response() for clarity.
            • Separated changes into distinct commits.
            • Added ui_change and docs_required.
            • Added upgrade.txt in improvement commit. (last one)
            • Agree with point 8 and function now always returns a string.
            • Added in rollback to transaction. Incased in try/catch.
            • Stuck with using a transaction, happy to chat about locks instead though.
            • Transaction no longer includes the events.

            Improvement only for Master, just pushing the 2 bugfix commits for 2.7 and 2.8.

            Show
            Zachary Durber added a comment - I've scrapped 1 & 2. Corrected placement of function description. Commented choice_user_submit_response() for clarity. Separated changes into distinct commits. Added ui_change and docs_required. Added upgrade.txt in improvement commit. (last one) Agree with point 8 and function now always returns a string. Added in rollback to transaction. Incased in try/catch. Stuck with using a transaction, happy to chat about locks instead though. Transaction no longer includes the events. Improvement only for Master, just pushing the 2 bugfix commits for 2.7 and 2.8.
            Hide
            Zachary Durber added a comment -

            Branch update.
            2.7 - Separate commit (because of backporting issues) that effectively has only the implementation of transactions in the commit to fix the synchronous database accessing problem. Doesn't require the second bugfix as limitations for choices wasn't broken in this code.

            2.8 - Backport of the first 2 commits on master. Transaction fix and limit fix. No improvements.

            2.9 - Both bugfixes and improvements to the choice_user_submit_response() function to return a string to notify the enduser of an invalid choice or limit being reached.

            Show
            Zachary Durber added a comment - Branch update. 2.7 - Separate commit (because of backporting issues) that effectively has only the implementation of transactions in the commit to fix the synchronous database accessing problem. Doesn't require the second bugfix as limitations for choices wasn't broken in this code. 2.8 - Backport of the first 2 commits on master. Transaction fix and limit fix. No improvements. 2.9 - Both bugfixes and improvements to the choice_user_submit_response() function to return a string to notify the enduser of an invalid choice or limit being reached.
            Hide
            CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-23273 using repository: git://github.com/zbdd/moodle.git

            More information about this report

            Show
            CiBoT added a comment - Fails against automated checks. Checked MDL-23273 using repository: git://github.com/zbdd/moodle.git MOODLE_27_STABLE (2 errors / 0 warnings) [branch: MDL-23273-27 | CI Job ] phplint (0/0) , php (2/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , MOODLE_28_STABLE (1 errors / 1 warnings) [branch: MDL-23273-28 | CI Job ] phplint (0/0) , php (1/1) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , master (2 errors / 1 warnings) [branch: MDL-23273-master | CI Job ] phplint (0/0) , php (2/1) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
            Hide
            CiBoT added a comment -

            Code verified against automated checks.

            Checked MDL-23273 using repository: git://github.com/zbdd/moodle.git

            More information about this report

            Show
            CiBoT added a comment - Code verified against automated checks. Checked MDL-23273 using repository: git://github.com/zbdd/moodle.git MOODLE_27_STABLE (0 errors / 0 warnings) [branch: MDL-23273-27 | CI Job ] MOODLE_28_STABLE (0 errors / 0 warnings) [branch: MDL-23273-28 | CI Job ] master (0 errors / 0 warnings) [branch: MDL-23273-master | CI Job ] More information about this report
            Hide
            Dan Marsden added a comment -

            shouldn't the update/insert records also sit inside the transaction? - and if we aren't using locks the check to see if limits are exceeded should come after the insert/update?

            Show
            Dan Marsden added a comment - shouldn't the update/insert records also sit inside the transaction? - and if we aren't using locks the check to see if limits are exceeded should come after the insert/update?
            Hide
            Zachary Durber added a comment -

            Yes this is true Dan, I found in testing the transaction as is was sufficient to stop the problem but I can imagine edge cases arising to replicate the original problem stated here. I'll have another go.

            Show
            Zachary Durber added a comment - Yes this is true Dan, I found in testing the transaction as is was sufficient to stop the problem but I can imagine edge cases arising to replicate the original problem stated here. I'll have another go.
            Hide
            Zachary Durber added a comment -

            Pushing back for a peer review.

            Changes:

            • transaction encompasses all the db actions and is then terminated before events are triggered.
            • print_errors in stable branches automatically dispose of the transaction, hence calling allow_commit() in the else loop.
              • (otherwise you get a transaction already disposes error in the catch)
            • most of the changes are just because of the indenting.
            Show
            Zachary Durber added a comment - Pushing back for a peer review. Changes: transaction encompasses all the db actions and is then terminated before events are triggered. print_errors in stable branches automatically dispose of the transaction, hence calling allow_commit() in the else loop. (otherwise you get a transaction already disposes error in the catch) most of the changes are just because of the indenting.
            Hide
            CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-23273 using repository: git://github.com/zbdd/moodle.git

            More information about this report

            Show
            CiBoT added a comment - Fails against automated checks. Checked MDL-23273 using repository: git://github.com/zbdd/moodle.git MOODLE_27_STABLE (3 errors / 3 warnings) [branch: MDL-23273-27 | CI Job ] phplint (0/0) , php (3/3) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , MOODLE_28_STABLE (1 errors / 1 warnings) [branch: MDL-23273-28 | CI Job ] phplint (0/0) , php (1/1) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , master (1 errors / 0 warnings) [branch: MDL-23273-master | CI Job ] phplint (0/0) , php (1/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
            Hide
            Zachary Durber added a comment - - edited

            I'll fix the above seeing as I'm touching the code anyway.

            Show
            Zachary Durber added a comment - - edited I'll fix the above seeing as I'm touching the code anyway.
            Hide
            CiBoT added a comment -

            Code verified against automated checks.

            Checked MDL-23273 using repository: git://github.com/zbdd/moodle.git

            More information about this report

            Show
            CiBoT added a comment - Code verified against automated checks. Checked MDL-23273 using repository: git://github.com/zbdd/moodle.git MOODLE_27_STABLE (0 errors / 0 warnings) [branch: MDL-23273-27 | CI Job ] MOODLE_28_STABLE (0 errors / 0 warnings) [branch: MDL-23273-28 | CI Job ] master (0 errors / 0 warnings) [branch: MDL-23273-master | CI Job ] More information about this report
            Hide
            Dan Marsden added a comment -

            Thanks Zachary, that's looking better although I'm still not sure if we're doing that round the right way. Putting it in a transaction should improve it but I don't think it's doing any locking is it? - so if 2 users start a transaction at exactly the same moment we could still end up with a problem?

            If we moved the code around a bit and inserted/updated the database first, then checked to see if the limits were exceeded and then rollback if the limits were exceeded would that be more reliable? - Ankit Agarwal what are your thoughts on that?

            Show
            Dan Marsden added a comment - Thanks Zachary, that's looking better although I'm still not sure if we're doing that round the right way. Putting it in a transaction should improve it but I don't think it's doing any locking is it? - so if 2 users start a transaction at exactly the same moment we could still end up with a problem? If we moved the code around a bit and inserted/updated the database first, then checked to see if the limits were exceeded and then rollback if the limits were exceeded would that be more reliable? - Ankit Agarwal what are your thoughts on that?
            Hide
            Ankit Agarwal added a comment -

            Apologies for delay in review as I was on holidays.

            1. First, I am not convinced this is the best control flow for the transaction. Try block should not have code after allow_commit() statement.

              try {
                   $transaction = $DB->start_delegated_transaction();
                   // Insert a record
                   $DB->insert_record('foo', $object);
                   $DB->insert_record('bar', $otherobject);
               
                   // Assuming the both inserts work, we get to the following line.
                   $transaction->allow_commit();
              } catch(Exception $e) {
                   $transaction->rollback($e);
              }
              

            The reason for this would be if the code after "allow_commit()" throws an exception, we would attempt a rollback of already committed transaction.

            1. Next, afaik Transactions come with inbuilt locks. But transactions are really designed for the rollback functionality imo, and using them as locks is not the ideal way. \core\lock\db_record_lock_factory() might be of help. Damyon Wiese might be able to suggest the best way to go forward with locks.
            2. Won't this api still return success in case an exception is thrown ?
            Show
            Ankit Agarwal added a comment - Apologies for delay in review as I was on holidays. First, I am not convinced this is the best control flow for the transaction. Try block should not have code after allow_commit() statement. try { $transaction = $DB->start_delegated_transaction(); // Insert a record $DB->insert_record('foo', $object); $DB->insert_record('bar', $otherobject); // Assuming the both inserts work, we get to the following line. $transaction->allow_commit(); } catch(Exception $e) { $transaction->rollback($e); } The reason for this would be if the code after "allow_commit()" throws an exception, we would attempt a rollback of already committed transaction. Next, afaik Transactions come with inbuilt locks. But transactions are really designed for the rollback functionality imo, and using them as locks is not the ideal way. \core\lock\db_record_lock_factory() might be of help. Damyon Wiese might be able to suggest the best way to go forward with locks. Won't this api still return success in case an exception is thrown ?
            Hide
            Zachary Durber added a comment -
            • Transactions are the right way to go, they will lock out the resource as we want. Confirmed this is the way to go with Damyon.
            • I re-arranged the code to support the transaction flow better by separating out the event recording and the database work.
            • If an exception is swallowed by the rollback, once it's disposes off the transaction it rethrows the exception.

            Going to backport the updated bugfix to 2.7 and 2.8 then send to Int Review.

            Show
            Zachary Durber added a comment - Transactions are the right way to go, they will lock out the resource as we want. Confirmed this is the way to go with Damyon. I re-arranged the code to support the transaction flow better by separating out the event recording and the database work. If an exception is swallowed by the rollback, once it's disposes off the transaction it rethrows the exception. Going to backport the updated bugfix to 2.7 and 2.8 then send to Int Review.
            Hide
            Dan Marsden added a comment -

            looks like you have some conflict markers in the latest comitted code too

            Show
            Dan Marsden added a comment - looks like you have some conflict markers in the latest comitted code too
            Hide
            Zachary Durber added a comment -

            Yep still in dev, accidentally updated tracker when I didn't mean to.

            Show
            Zachary Durber added a comment - Yep still in dev, accidentally updated tracker when I didn't mean to.
            Hide
            CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-23273 using repository: git://github.com/zbdd/moodle.git

            More information about this report

            Show
            CiBoT added a comment - Fails against automated checks. Checked MDL-23273 using repository: git://github.com/zbdd/moodle.git MOODLE_27_STABLE (20 errors / 0 warnings) [branch: MDL-23273-27 | CI Job ] phplint (1/0) , php (19/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , MOODLE_28_STABLE (3 errors / 0 warnings) [branch: MDL-23273-28-newfix | CI Job ] phplint (0/0) , php (3/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , master (3 errors / 0 warnings) [branch: MDL-23273-master | CI Job ] phplint (0/0) , php (3/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
            Hide
            David Monllaó added a comment -

            Thanks for your contribution, this change is now part of Moodle and is available from git.moodle.org and shortly on https://download.moodle.org

            Expect problems and eat them for breakfast.

            • Alfred A. Montapert
            Show
            David Monllaó added a comment - Thanks for your contribution, this change is now part of Moodle and is available from git.moodle.org and shortly on https://download.moodle.org Expect problems and eat them for breakfast. Alfred A. Montapert
            Hide
            David Monllaó added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.
            TIA and ciao

            PD: Forget about the last comment, it was a copy & paste error. This issue's status hasn't change and it is still waiting for integration.

            Show
            David Monllaó added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao PD: Forget about the last comment, it was a copy & paste error. This issue's status hasn't change and it is still waiting for integration.
            Hide
            CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            CiBoT added a comment -

            Code verified against automated checks.

            Checked MDL-23273 using repository: git://github.com/zbdd/moodle.git

            More information about this report

            Show
            CiBoT added a comment - Code verified against automated checks. Checked MDL-23273 using repository: git://github.com/zbdd/moodle.git MOODLE_27_STABLE (0 errors / 0 warnings) [branch: MDL-23273-27 | CI Job ] MOODLE_28_STABLE (0 errors / 0 warnings) [branch: MDL-23273-28-newfix | CI Job ] master (0 errors / 0 warnings) [branch: MDL-23273-master | CI Job ] More information about this report
            Hide
            David Monllaó added a comment -

            Sorry Zac, thanks for your work here (and thanks to all the reviewers too) but I am reopening this, there are a few things to fix before this is ready:

            • I've been reading http://www.postgresql.org/docs/9.1/static/transaction-iso.html#XACT-READ-COMMITTED, I understand that if a transaction inserts/updates any record, the 2nd transaction select (or any other transaction) will wait until these changes are committed, but as far as I understand, this does not mean that once a transaction is started another transaction can not start, they can both get the same number of choice answers, $choicesexceeded would be 0 in both cases and they will both insert into DB, in fact I've tried hacking choice_user_submit_response() adding sleep here and there and I've been able to insert both records. We should re-think about the function execution flow and rollback if we passed the limit.
            • The triggered event changes depending on if it is a new answer or an answer is being updated, you are just calling \mod_choice\event\answer_submitted
            • There is a failing unit test

              1) mod_choice_events_testcase::test_answer_updated
              Failed asserting that mod_choice\event\answer_submitted Object (...) is an instance of class "\mod_choice\event\answer_updated".
               
              /home/davidm/Desktop/moodlecode/int/master/mod/choice/tests/events_test.php:162
              /home/davidm/Desktop/moodlecode/int/master/lib/phpunit/classes/advanced_testcase.php:80
               
              To re-run:
               vendor/bin/phpunit mod_choice_events_testcase mod/choice/tests/events_test.php
              

            • What kind of exceptions do you expect to catch? I don't think it is necessary; in any case, if you are catching db errors you might want to look for dml_exception so other possible exceptions triggered during the process will still stop the execution
            • In master, you are changing 2 print_error() to return $string; https://github.com/zbdd/moodle/compare/88cd577...MDL-23273-master#diff-c3eb8f93fb42c0b7a1210ebf0f093b5fR256, print_error() stops execution after that point and now we continue, any reason for this change? It seems that in these cases the "exception" is important enough to just stop the execution.
            • In case I am missing something and the transaction start is even blocking other reads to be executed, could you start_delegated_transaction() just before if($choice->limitanswers)? it does not seem that we will need any block to get our own current choice answers
            • To ensure that the issue is properly tested you could ask the tester to add a sleep(N) in a couple of different points in choice_user_submit_response() so we ensure that we test what is happening when 2 limited options are selected at the same time
            • Also about testing instructions, as we deal with transactions would be worth to test it in multiple dbs. ATM there is no decision about MDL-46064 (I guess/expect that MyISAM will be dropped) but for people using MyISAM this will not help.
            Show
            David Monllaó added a comment - Sorry Zac, thanks for your work here (and thanks to all the reviewers too) but I am reopening this, there are a few things to fix before this is ready: I've been reading http://www.postgresql.org/docs/9.1/static/transaction-iso.html#XACT-READ-COMMITTED , I understand that if a transaction inserts/updates any record, the 2nd transaction select (or any other transaction) will wait until these changes are committed, but as far as I understand, this does not mean that once a transaction is started another transaction can not start, they can both get the same number of choice answers, $choicesexceeded would be 0 in both cases and they will both insert into DB, in fact I've tried hacking choice_user_submit_response() adding sleep here and there and I've been able to insert both records. We should re-think about the function execution flow and rollback if we passed the limit. The triggered event changes depending on if it is a new answer or an answer is being updated, you are just calling \mod_choice\event\answer_submitted There is a failing unit test 1) mod_choice_events_testcase::test_answer_updated Failed asserting that mod_choice\event\answer_submitted Object (...) is an instance of class "\mod_choice\event\answer_updated".   /home/davidm/Desktop/moodlecode/int/master/mod/choice/tests/events_test.php:162 /home/davidm/Desktop/moodlecode/int/master/lib/phpunit/classes/advanced_testcase.php:80   To re-run: vendor/bin/phpunit mod_choice_events_testcase mod/choice/tests/events_test.php What kind of exceptions do you expect to catch? I don't think it is necessary; in any case, if you are catching db errors you might want to look for dml_exception so other possible exceptions triggered during the process will still stop the execution In master, you are changing 2 print_error() to return $string; https://github.com/zbdd/moodle/compare/88cd577...MDL-23273-master#diff-c3eb8f93fb42c0b7a1210ebf0f093b5fR256 , print_error() stops execution after that point and now we continue, any reason for this change? It seems that in these cases the "exception" is important enough to just stop the execution. In case I am missing something and the transaction start is even blocking other reads to be executed, could you start_delegated_transaction() just before if($choice->limitanswers) ? it does not seem that we will need any block to get our own current choice answers To ensure that the issue is properly tested you could ask the tester to add a sleep(N) in a couple of different points in choice_user_submit_response() so we ensure that we test what is happening when 2 limited options are selected at the same time Also about testing instructions, as we deal with transactions would be worth to test it in multiple dbs. ATM there is no decision about MDL-46064 (I guess/expect that MyISAM will be dropped) but for people using MyISAM this will not help.
            Hide
            CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            Dan Marsden added a comment -

            Thanks for confirming that David - I suspected that myself too.

            The calls could be reversed within the transaction - insert record then check if limits exceeded as the limit is initially checked on the page the user views and would roll-back - but this could only work if transactions are supported on the current db. If transactions are not supported the order of operations should be the other way round - (check limits then insert if possible) - this means you may need to check if transactions are supported and then do it one way or the other. Hopefully you can exclude this logic in the master version of the code if MDL-46064 gets approved.

            Show
            Dan Marsden added a comment - Thanks for confirming that David - I suspected that myself too. The calls could be reversed within the transaction - insert record then check if limits exceeded as the limit is initially checked on the page the user views and would roll-back - but this could only work if transactions are supported on the current db. If transactions are not supported the order of operations should be the other way round - (check limits then insert if possible) - this means you may need to check if transactions are supported and then do it one way or the other. Hopefully you can exclude this logic in the master version of the code if MDL-46064 gets approved.
            Hide
            Zachary Durber added a comment -

            Sorry about that guys, I'm having a fresh look at it.
            I'm looking at combining the double checking aspect in the patch by Claus and using transactions around the update statement.
            Something like this:
            -> check limit
            -> limit okay continue
            -> transaction start
            -> insert/update
            -> transaction end
            -> check limit
            -> write event logs

            Rather than write a new function to handle double checking, I'm wrapping up all the extra limit checking code already within the code into a separate function to re-use.

            Show
            Zachary Durber added a comment - Sorry about that guys, I'm having a fresh look at it. I'm looking at combining the double checking aspect in the patch by Claus and using transactions around the update statement. Something like this: -> check limit -> limit okay continue -> transaction start -> insert/update -> transaction end -> check limit -> write event logs Rather than write a new function to handle double checking, I'm wrapping up all the extra limit checking code already within the code into a separate function to re-use.
            Hide
            Dan Marsden added a comment -

            that seems quite heavy in the case where we are trying to mitigate perf issues to me. This bug occurs when server is under high load. Implementing a solution that causes a higher load isn't really ideal.

            I would suggest:
            If transaction support
            ->transaction start
            -> insert/update
            -> check limit - if this fails rollback transaction.
            -> transaction end
            else If no Transaction support
            ->check limit
            -> insert/update
            -> (optional) check limit again

            If transaction support isn't there we could either check the limit again or keep existing behaviour and state that you must have transaction support to avoid this bug.

            The initial view choice page performs a limit check before allowing the user to save the choice - in transaction mode I think it is safe enough to put the insert first and check the limits after insert rather than causing a more significant load on the server at the time when we really need it to be fast.

            Show
            Dan Marsden added a comment - that seems quite heavy in the case where we are trying to mitigate perf issues to me. This bug occurs when server is under high load. Implementing a solution that causes a higher load isn't really ideal. I would suggest: If transaction support ->transaction start -> insert/update -> check limit - if this fails rollback transaction. -> transaction end else If no Transaction support ->check limit -> insert/update -> (optional) check limit again If transaction support isn't there we could either check the limit again or keep existing behaviour and state that you must have transaction support to avoid this bug. The initial view choice page performs a limit check before allowing the user to save the choice - in transaction mode I think it is safe enough to put the insert first and check the limits after insert rather than causing a more significant load on the server at the time when we really need it to be fast.
            Hide
            Zachary Durber added a comment - - edited

            Hi Dan, I played more with transactions and they just don't lock the resource out as expected in the end. That and any double checking after an insert (excluding rollbacks) destroys performance on the page.

            After going back to the drawing board I opted for locks and just re-arranged some of the code. i.e. the event recording so only the get/insert statements require a lock.
            The additional benefit here is that locks are supported from 2.7 onwards and we don't have to worry about lack of db support for transactions.

            This has passed all my tests to breach the limit and I'm happy with the speed overall. (1000 students submitting at once for a single choice with 5 options. (limits 1, 2, 3, 4, 5) takes 14 seconds on average.)

            I've removed the warning improvement, if this is desired we can spin a new story for it but as it stands implementing the bugfix is more important than continuing to tweak the code.

            Show
            Zachary Durber added a comment - - edited Hi Dan, I played more with transactions and they just don't lock the resource out as expected in the end. That and any double checking after an insert (excluding rollbacks) destroys performance on the page. After going back to the drawing board I opted for locks and just re-arranged some of the code. i.e. the event recording so only the get/insert statements require a lock. The additional benefit here is that locks are supported from 2.7 onwards and we don't have to worry about lack of db support for transactions. This has passed all my tests to breach the limit and I'm happy with the speed overall. (1000 students submitting at once for a single choice with 5 options. (limits 1, 2, 3, 4, 5) takes 14 seconds on average.) I've removed the warning improvement, if this is desired we can spin a new story for it but as it stands implementing the bugfix is more important than continuing to tweak the code.
            Hide
            CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-23273 using repository: git://github.com/zbdd/moodle.git

            More information about this report

            Show
            CiBoT added a comment - Fails against automated checks. Checked MDL-23273 using repository: git://github.com/zbdd/moodle.git MOODLE_27_STABLE (3 errors / 0 warnings) [branch: MDL-23273-27 | CI Job ] phplint (0/0) , php (3/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , MOODLE_28_STABLE (0 errors / 0 warnings) [branch: MDL-23273-28 | CI Job ] master (0 errors / 0 warnings) [branch: MDL-23273-master | CI Job ] More information about this report
            Hide
            Ankit Agarwal added a comment -

            Thanks Zac,
            Locks has been my preferred solution from start. The patch looks good. A few minor things:-

            1. Typo - synchonous
            2. moodle_exception should provide a better error message . The error "The operation timed out while waiting for a lock." means nothing to an user who is trying to submit a a choice response.

            Rest looks good,
            Thanks

            Show
            Ankit Agarwal added a comment - Thanks Zac, Locks has been my preferred solution from start. The patch looks good. A few minor things:- Typo - synchonous moodle_exception should provide a better error message . The error "The operation timed out while waiting for a lock." means nothing to an user who is trying to submit a a choice response. Rest looks good, Thanks
            Hide
            Zachary Durber added a comment -

            Thanks Ankit, made the requested changes. Pushing for Integration Review.

            Show
            Zachary Durber added a comment - Thanks Ankit, made the requested changes. Pushing for Integration Review.
            Hide
            CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            CiBoT added a comment -

            Code verified against automated checks.

            Checked MDL-23273 using repository: git://github.com/zbdd/moodle.git

            More information about this report

            Show
            CiBoT added a comment - Code verified against automated checks. Checked MDL-23273 using repository: git://github.com/zbdd/moodle.git MOODLE_27_STABLE (0 errors / 0 warnings) [branch: MDL-23273-27 | CI Job ] MOODLE_28_STABLE (0 errors / 0 warnings) [branch: MDL-23273-28 | CI Job ] master (0 errors / 0 warnings) [branch: MDL-23273-master | CI Job ] More information about this report
            Hide
            Eloy Lafuente (stronk7) added a comment - - edited

            I agree locks is the way for this 100% (was a bit confused while reading about transactions above, not being able to imagine how they could help). Anyway, IMO we just must ensure:

            • Is the lock is released always? I've looked to current implementations and all them seem to have supports_auto_release=true, but just in case anybody is using a lock not having such feature (not common, agree, but still) we must ensure that release_lock() is being used in all the exit points in the script.
            • Would be really interesting to get results both before and after the patch. I've seen you tried above with 1000 submissions and it became completed in 14 seconds... pretty impressive... did you look how many "cannotsubmit" you got? Or were the 1000 processed without a problem (ending saved as 1000 submissions?).
            • Perhaps, only perhaps... I'd apply the locks only when there are limits to check. If the choice is a "normal" one we should not be applying for any lock.
            • Edited: Does the error provide a link back to the choice? IMO that would be the expected behavior.

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - - edited I agree locks is the way for this 100% (was a bit confused while reading about transactions above, not being able to imagine how they could help). Anyway, IMO we just must ensure: Is the lock is released always? I've looked to current implementations and all them seem to have supports_auto_release=true, but just in case anybody is using a lock not having such feature (not common, agree, but still) we must ensure that release_lock() is being used in all the exit points in the script. Would be really interesting to get results both before and after the patch. I've seen you tried above with 1000 submissions and it became completed in 14 seconds... pretty impressive... did you look how many "cannotsubmit" you got? Or were the 1000 processed without a problem (ending saved as 1000 submissions?). Perhaps, only perhaps... I'd apply the locks only when there are limits to check. If the choice is a "normal" one we should not be applying for any lock. Edited: Does the error provide a link back to the choice? IMO that would be the expected behavior. Ciao
            Hide
            Zachary Durber added a comment - - edited

            Hi Eloy,

            • I'll double check, but I believe the lock is always released. (I've placed exit; and similar through out the code during testing) - as if locks are not supported the timeout will release the lock.
            • That old benchmark was when i was using transactions, the same test comes in at under 4 seconds for 1000 students. I was also recording failed and successful attempts. (Saved: 6 and Stopped: 994). All processed fine, the stopped is when an exception is thrown (by print_error() - usually when the limit has been exceeded)
            • I've updated the code to only apply the lock when limits are used.
            • The error never provided a way to return to the choice directly, luckily that is easy to add as print_error's 3rd argument lets me supply a URL to the continue button.

            I'm appending an additional commit now with the suggested changes.

            Show
            Zachary Durber added a comment - - edited Hi Eloy, I'll double check, but I believe the lock is always released. (I've placed exit; and similar through out the code during testing) - as if locks are not supported the timeout will release the lock. That old benchmark was when i was using transactions, the same test comes in at under 4 seconds for 1000 students. I was also recording failed and successful attempts. (Saved: 6 and Stopped: 994). All processed fine, the stopped is when an exception is thrown (by print_error() - usually when the limit has been exceeded) I've updated the code to only apply the lock when limits are used. The error never provided a way to return to the choice directly, luckily that is easy to add as print_error's 3rd argument lets me supply a URL to the continue button. I'm appending an additional commit now with the suggested changes.
            Hide
            Dan Poltawski added a comment -

            Sorry Zac I didn't get back to this in time for this week.

            The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.
            This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.
            This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P
            Apologies for the inconvenience, this will be integrated next week. Thanks for your collaboration & ciao

            Show
            Dan Poltawski added a comment - Sorry Zac I didn't get back to this in time for this week. The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologies for the inconvenience, this will be integrated next week. Thanks for your collaboration & ciao
            Hide
            Dan Poltawski added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.
            TIA and ciao

            Show
            Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            CiBoT added a comment -

            Code verified against automated checks.

            Checked MDL-23273 using repository: git://github.com/zbdd/moodle.git

            More information about this report

            Show
            CiBoT added a comment - Code verified against automated checks. Checked MDL-23273 using repository: git://github.com/zbdd/moodle.git MOODLE_27_STABLE (0 errors / 0 warnings) [branch: MDL-23273-27 | CI Job ] MOODLE_28_STABLE (0 errors / 0 warnings) [branch: MDL-23273-28 | CI Job ] master (0 errors / 0 warnings) [branch: MDL-23273-master | CI Job ] More information about this report
            Hide
            Dan Poltawski added a comment -

            Thanks Zac, integrated to master, 28 and 27

            Show
            Dan Poltawski added a comment - Thanks Zac, integrated to master, 28 and 27
            Hide
            Rajesh Taneja added a comment -

            Thanks for working on this Zac,

            It works as expected, got an error message for 2nd student attempt.

            Although, it doesn't look right to show error message. It would be nice to have a notification or reload choice page with notification saying "Option is full/unavailable"

            As discussed here, it will be handled by a follow-up issue, so passing...

            Show
            Rajesh Taneja added a comment - Thanks for working on this Zac, It works as expected, got an error message for 2nd student attempt. Although, it doesn't look right to show error message. It would be nice to have a notification or reload choice page with notification saying "Option is full/unavailable" As discussed here, it will be handled by a follow-up issue, so passing...
            Hide
            Eloy Lafuente (stronk7) added a comment -

            And this is now part of Moodle and, soon, part of the life of some millions of people out there. Amazing! Well done!

            Consensus isn't just about agreement. It's about changing things around:
            You get a proposal, you work something out, people foresee problems, you
            do creative synthesis. At the end of it, you come up with something that
            everyone thinks is okay. Most people like it, and nobody hates it.
            ---- David Graeber

            Show
            Eloy Lafuente (stronk7) added a comment - And this is now part of Moodle and, soon, part of the life of some millions of people out there. Amazing! Well done! Consensus isn't just about agreement. It's about changing things around: You get a proposal, you work something out, people foresee problems, you do creative synthesis. At the end of it, you come up with something that everyone thinks is okay. Most people like it, and nobody hates it. ---- David Graeber
            Hide
            Helen Foster added a comment -

            This issue is labelled docs_required; however, it seems it's a bug fix rather than a new feature or improvement. Please can anyone specify what needs documenting; otherwise the docs_required label can be removed.

            Show
            Helen Foster added a comment - This issue is labelled docs_required; however, it seems it's a bug fix rather than a new feature or improvement. Please can anyone specify what needs documenting; otherwise the docs_required label can be removed.
            Hide
            Helen Foster added a comment -

            Assuming it's OK to remove the docs_required label... If I'm wrong, please comment and let me know.

            Show
            Helen Foster added a comment - Assuming it's OK to remove the docs_required label... If I'm wrong, please comment and let me know.

              People

              • Votes:
                11 Vote for this issue
                Watchers:
                22 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Agile