Quiz re-grades should ignore_user_abort and unlock the session before starting

Can anyone give me some clues in fixing it as I'm still getting familiar with Moodle? I'm supposing this bug is related to files under moodle/grade/*? Thanks!

This bug relates to code under mod/quiz. Until you know what re-grading is, you won't get anywhere.

How familiar are you with web application, specifically in PHP. Do you know

• what the session is?
• why it might normally be locked?

Once you have the necessary background, then we can talk about fixing this bug.

Hi Tim, Thanks so much for the information!

I finally navigated to the source file moodle/mod/quiz/overview/report.php for the regrading and found out following functions related to the process of regarding:

Following are the functions responsible for the regrading:

1) process_actions($quiz, $cm, $currentgroup, $groupstudents, $allowed, $redirecturl)
2) regrade_attempt($attempt, $dryrun = false, $slots = null)
3) regrade_attempts($quiz, $dryrun = false, $groupstudents = array(), $attemptids = array())
4) regrade_attempts_needing_it($quiz, $groupstudents)
5) count_question_attempts_needing_regrade($quiz, $groupstudents)
6) has_regraded_questions($from, $where, $params)
7) clear_regrade_table($quiz, $groupstudents)
8) update_overall_grades($quiz)

And the first two should be the most important ones. The first one schedules the regrading requests to corresponding function calls and the second one carries out the exact regarding process.

---------------------------------------------------------------------------------

Regarding the second part of your comments, I don't have that much experience in web applications. The only project I finished recently is last year's GSoC, the complete pronunciation evaluation website. It can be tried at http://talknicer.net/~li-bo/datacollection/login.php. However, as we are moving the sites to a new server the link is currently not working. I will update the link once the migration is done. For now, if you are interested, the sources could be found at http://sourceforge.net/p/cmusphinx/code/HEAD/tree/branches/speecheval/troy/.

Regarding the question "what the session is". As I have never studies PHP in a systematical way, I only know it keeps track of a user's information during his/her visiting of the website. And the only thing I have touched session in PHP is during last summer's GSoC.
a) After a user logs in, the session_start() is called;
b) $_SESSION['variable']=value is used when I need to pass over some variables between different pages;
c) session_destroy() is called when the user logs out.
That's all I know about sessions in PHP.

Regarding the second question "why it might normally lock?", I have no idea. But with the help of Google, I learned following aspects:

From http://konrness.com/php5/how-to-prevent-blocking-php-requests/, the session locks due to multiple requests to access the same session file and could be solved by closing the session with session_write_close() before starting the long running process. This function only close the write session capability, the session variables are still readable.

However, in my opinion it is hard to make sure there will never be write requests for the session variables.

Just let me know whether I'm heading to the correct direction or not and any comments/suggestions are welcome. Thanks!

Hi Tim, currently I'm thinking of whether we can solve the bug in the following way:

In the process_actions() function, to surround the original regrading function calls with:

----------------------------------------------------------

$olduserabort=ignore_user_abort();
$oldtimelimit=ini_get('max_execution_time');

ignore_user_abort(true);
set_time_limit(0);
session_write_close();

[... original regrading function calls ...]

ignore_user_abort($olduserabort);
set_time_limit($oldtimelimit);

-----------------------------------------------------

Just let me know your suggestions. Thanks!

PS. I found a discussion on why session_write_close() is not used in the existing Moodle code (https://moodle.org/mod/forum/discuss.php?d=16988), but there seems nobody saying it is not allowed. Do let me know if there are any reason to avoid it.

That forum thread is relevant, despite the date. It is saying that we should unlock the session when relevant. The Moodle API has moved on since then. You can see the right way to unlock the session in the top-level script file.php that handles file downloads.

You don't need to worry about setting ignore_user_abort or the time limit back to the original value. All the regrade actions end with a redirect(). That function never returns. I redirects the user to another URL.

We probably don't want to set_time_limit(0); No limit is dangerous. What if some buggy question type causes an infinite loop? Instead, we want an allowance like there is already in the regrade_attempt method.

The problem originally reported here is that the time limit is not sufficient when there are a lot of questions in the quiz.

Are you saying we need to set a different time limit for different attempt based on the number of questions in that attempt? If so, should we take the question types into consideration as grading a math problem is usually different from an essay problem?

No, that is too complex. I think just bump that number up (perhaps search the whole codebase for set_time_limit to see what values are typically used elsewehre) and then add the session stuff.

Hi Tim, thanks so much for your suggestion! I have done the changes which could be found at https://github.com/troylee/moodle/tree/wip-MDL-36020-master . It is based on the current master branch. As it is my first time doing this, I prefer to apply the changes to other branches until it is confirmed correct.

Just find on GitHub the changes could be clearly seen at https://github.com/troylee/moodle/compare/wip-MDL-36020-master .

From your blog post http://tjhunt.blogspot.sg/2012/03/fixing-bug-in-moodle-core-mechanics.html, I should actually submit a Peer review request form rather than putting the link in the comment. However, I cannot find it or the "workflow" button mentioned in the development docs. Do let me know what's the correct process. Thanks!

Sorry for the delay in reviewing this.

Only people with 'developer' permssions can see the 'Request peer review' button, and users are normally only given developer permissions after they have fixed a few bugs. Until then, adding comments to explain what is going on, is the right thing to do.

Now for the actual review:

I think the patch is now correct, in that it fixes the problem.

However, I wonder if it is the best possible patch. It is a pity to duplicate the lines

ignore_user_abort(true);
session_get_instance()->write_close();

four times. I can think of two ways around that:
1. Make a function called something like unlock_session, so then we only have one line of code duplicated.
2. Move those lines inside regrade_attempts and regrade_attempts_needing_it so they are only duplicated twice.
Do you think either of those are worth doing?

There is also one small coding style problem:

// unlock session before regrading 

should be

// Unlock session before regrading.

However, I am not sure that the comments add much. A line of code like $this->unlock_session might be self-documenting.

Would you like to prepared a revised commit?

Thank you so much for the review and suggestions!

I have put them into a new function unlock_session() in the quiz_overview_report class. I initially thought of putting it into the more general session classes in the lib/sessionlib.php file. However, as it simply adds the ignore_user_abort(true) configuration to the existing write_close() function, it may not be a basic operation that many others will use. Hence, I decided to put it in the quiz_overview_report class. Besides, I have also used the code checker to ensure my coding style is correct.

Please find the changes at https://github.com/troylee/moodle/compare/wip-MDL-36020-master.

Do let me know if you have any comments or suggestions!

The overall change is now looking good. I think the final thing we need to do is to re-base it to turn it into a single commit. If you don't know how to do this already, either ask for help, or read the docs about git rebase -i.

Thanks so much for your review! To re-base the changes, what I have done is as follows:

1) update my repository with the remote moodle repository by:
git fetch --all --prune

2) update my local master branch:
git checkout master
git pull
git push origin master

3) re-base the bug fixing branch:
git checkout wip-MDL-36020-master
git rebase origin/master

4) push to my github account:
git push -f origin wip-MDL-36020-master

And then the final changes could be found at https://github.com/troylee/moodle/compare/wip-MDL-36020-master.

Those steps are I found in the Moodle development docs, just let me know if there are any problem! Thank you!

That is half of what is necessary (re-basing on top of the latest weekly build). However, if you look at https://github.com/troylee/moodle/compare/wip-MDL-36020-master, you will see that the change still comprises 5 separate commits.

git rebase -i

will let you combine (or squash) them into a single commit. You need to do that.

Thanks for point it out and I finally figured out the following steps:

1) git rebase -i HEAD~5

2) In the opened editor leave one commit as "pick" and change all the other to "squash". Then exit and save.

3) Edit the opened commit message if necessary.

4) Push to my githut account
git push -f origin wip-MDL-36020-master

Then the current commit at https://github.com/troylee/moodle/compare/wip-MDL-36020-master has only 1 commit.

Just let me know if these steps are correct! Thank you!

Yes. that is right. At step 1) you could also type "git rebase -i master" (since when you started HEAD~5 and master pointed to the same commit).

We need one final thing before this can be submitted for integration. We need testing instructions, that explain to another human how to test this change to verify

1. It fixes the stated problem; and
2. It does not break anything else (within reason).

Do you want to have a go at writing some testing instructions. (Try looking at some of the other issues currently in integration, to get a feel for what people write, https://tracker.moodle.org/secure/Dashboard.jspa?selectPageId=11350. Testing instructions normally go in a special field near the top of the bug. You probably don't have permission to edit that field, so write your testing instructions in a comment, and I can move them.)

Hi Tim, thanks so much for all your guidance! The test instructions I have drafted are as follows:

1) Create a quiz that requires longer grading time (for example with lots of questions).

2) Log in the system using different student test users to complete the quiz.

3) Log in as a teacher and navigate to Quiz administration -> Results page, click the "Regrade all" or "Regrade selected attempts" button and before it finishes, do other things such as open other pages etc. Check whether those operations can be carried out during the regrading process.

Let me know if there are any comments! Thanks!

To INTEGRATORS:

Should this be back-ported to stable branches? Probably yes. Please can you cherry-pick it?

Integrated to master, 25, 24 and 23.

I agree that the session locking was a bug which is useful for improving performance of sites on all the branches, hence the backport. I probably should've insisted upon a backport request, so remember this one

Hi Dan, Thank you so much for the integration! It's great to see my first attempt on the Moodle open source project being accepted!

Hi Troy,

Its a pleasure, thanks a lot for contributing your patch to Moodle!

Please keep the patches coming!

I did not have enough data to get longer processing times, so I hacked it with some sleep() statements instead, works fine for me in all branches. Thanks.

Thanks for the testing! I will try my best to contribute more!

Thanks for your contribution! This issue has been reviewed, integrated, tested and now released to everyone.

Closing as Fixed!

Thank you all for the guidance, review, integration and testing! I'd love to contribute more!