Moodle

Silent Loss of Session Data!

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 1.9.3
  • Fix Version/s: 1.9.10
  • Component/s: SCORM
  • Labels:
    None
  • Environment:
    Debian Linux
  • Database:
    MySQL
  • Difficulty:
    Moderate
  • Affected Branches:
    MOODLE_19_STABLE
  • Fixed Branches:
    MOODLE_19_STABLE

Description

In /mod/scorm/datamodels/scorm_12js.php, method LMSFinish() on line 185 writes back the session data through the StoreData() function. If successful, StoreData() returns the string "true". Currently the result of this call is captured but unhandled. Just below where it is called there is a hard coded return "true"; statement. So if StoreData() fails for whatever reason (proxy auth issue, network traffic, firewall block) in its HttpRequest attempt then all the student's work will be silently lost. That's a rather serious issue when it comes to online learning, and it's just happened to me several times during testing.

I raised this issue once before nearly a year ago in http://tracker.moodle.org/browse/MDL-13716 (which has since been reviewed and closed)

Attachments

  1. File
    LMSFinish.php
    02/Apr/09 7:23 AM
    1 kB
    Jaime Liz
  2. Text File
    patch.txt
    02/Apr/09 7:53 AM
    0.7 kB
    Jaime Liz
  3. File
    scorm_12.js.php
    04/Mar/10 7:06 AM
    32 kB
    Ron Meske

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Piers Harding added a comment -

Hi Peter - having a look at this I notice that StoreData() returns the correct thing if the HTTP call succeeds (either true or false), but would in effect return nothing or garbage in the HTTP call failed. It is easy to fix the return from LMSFinish() to just return the result of StoreData(), but what should the correct behaviour be in the case of an HTTP failure - "false" ?

Cheers,
Piers Harding.

Show
Piers Harding added a comment - Hi Peter - having a look at this I notice that StoreData() returns the correct thing if the HTTP call succeeds (either true or false), but would in effect return nothing or garbage in the HTTP call failed. It is easy to fix the return from LMSFinish() to just return the result of StoreData(), but what should the correct behaviour be in the case of an HTTP failure - "false" ? Cheers, Piers Harding.
Hide
Permalink
Peter Chamberlin added a comment -

LMSFinish() should return string "true" on success and string "false" for any other state, such as if additional API calls cannot be guaranteed to process correctly. If the HTTP call failed then I'd consider that a failure of LMSFinish as the SCO cannot be assured that data set with LMSSetValue() has been persisted. I believe the appropriate SCORM 1.2 error code would be "101" (General Exception). That would also allow me to give warning to the user that all their work has just been lost. It may be worth keeping the SCO<>LMS state connected in such a failure so that the SCO has the opportunity to retry LMSFinish(), or putting a multi-attempt fallback mechanism into the way session data is fed back to the database to handle situations of high load or temporary connectivity loss.

Show
Peter Chamberlin added a comment - LMSFinish() should return string "true" on success and string "false" for any other state, such as if additional API calls cannot be guaranteed to process correctly. If the HTTP call failed then I'd consider that a failure of LMSFinish as the SCO cannot be assured that data set with LMSSetValue() has been persisted. I believe the appropriate SCORM 1.2 error code would be "101" (General Exception). That would also allow me to give warning to the user that all their work has just been lost. It may be worth keeping the SCO<>LMS state connected in such a failure so that the SCO has the opportunity to retry LMSFinish(), or putting a multi-attempt fallback mechanism into the way session data is fed back to the database to handle situations of high load or temporary connectivity loss.
Hide
Permalink
Jaime Liz added a comment -

I have just changed the return statement to return the result of the DataStore function if it has been executed or just FALSE if it didn't get to be executed within the LMSFinish fuction

Show
Jaime Liz added a comment - I have just changed the return statement to return the result of the DataStore function if it has been executed or just FALSE if it didn't get to be executed within the LMSFinish fuction
Hide
Permalink
Jaime Liz added a comment -

here's the patch file Dan xD

Show
Jaime Liz added a comment - here's the patch file Dan xD
Hide
Permalink
Dan Marsden added a comment -

Hi Peter - do you think the path Jamie attached here is adequate? - or should we be returning an errorcode instead?

Show
Dan Marsden added a comment - Hi Peter - do you think the path Jamie attached here is adequate? - or should we be returning an errorcode instead?
Hide
Permalink
Peter Chamberlin added a comment -

It is adequate, as knowing about data write-back failure is the main thing when the communications layer fails. That can then trigger investigation into the cause. Addition error information would then be useful in debugging mode.

Show
Peter Chamberlin added a comment - It is adequate, as knowing about data write-back failure is the main thing when the communications layer fails. That can then trigger investigation into the cause. Addition error information would then be useful in debugging mode.
Hide
Permalink
Dan Marsden added a comment -

Thanks Peter - fix pushed into HEAD - can you please check it out before I push to stable?

http://cvs.moodle.org/moodle/mod/scorm/datamodels/scorm_12.js.php?r1=1.23&r2=1.24

Show
Dan Marsden added a comment - Thanks Peter - fix pushed into HEAD - can you please check it out before I push to stable? http://cvs.moodle.org/moodle/mod/scorm/datamodels/scorm_12.js.php?r1=1.23&r2=1.24
Hide
Permalink
Dan Marsden added a comment -

just following up - has anyone been able to test and verify this fix works on their 1.9 install?

thanks,

Show
Dan Marsden added a comment - just following up - has anyone been able to test and verify this fix works on their 1.9 install? thanks,
Hide
Permalink
Matteo Scaramuccia added a comment -

Hi Dan,
I cannot test it right now but I want to highlight a possible issue regarding with the fix submitted in HEAD i.e. just looking at the code flow. Please, find below the comment at the fix and our proposal for the discussion.

result is now proposed to be evaluated through the return value of StoreData; this return value is built with this logic below:

result = DoRequest(myRequest,"<?php p($CFG->wwwroot) ?>/mod/scorm/datamodel.php","id=<?php p($id) ?>&sesskey=<?php p($USER->sesskey) ?>"+datastring);
        results = String(result).split('\n');
        errorCode = results[1];
        return results[0];

Everything will work as expected (i.e. true or false) until the AJAX call will fail:

if (httpReq.status == 200) {
        //popupwin(url+"\n"+param+"\n"+httpReq.responseText);
        return httpReq.responseText;
    } else {
        return httpReq.status;
    }

so it could be IMHO better to code ''true if true, false otherwise'', e.g.:

@@ -199,13 +199,20 @@
                         setTimeout('top.document.location=top.next;',500);
                     }
                 }
+                <?php
+                    if (debugging('',DEBUG_DEVELOPER)) {
+                        echo 'LogAPICall("LMSFinish", "AJAXResult", result, 0);';
+                    }
+                ?>
+                result = ('true' == result ? 'true' : 'false');
                 <?php
                     if (debugging('',DEBUG_DEVELOPER)) {
                         //echo 'alert("Finished SCORM 1.2");';
+                        echo 'LogAPICall("LMSFinish", "result", result, 0);';
                         echo 'LogAPICall("LMSFinish", param, "", 0);';
                     }
                 ?>
-                return "true";
+                return result;
             } else {
                 errorCode = "301";
             }

Note: I've rearranged also the LogAPICall parameters order, according to the signature&logic found in scorm_12.js.php.
Note: we're interested in the fix of this issue also in 1.9 branch.

Show
Matteo Scaramuccia added a comment - Hi Dan, I cannot test it right now but I want to highlight a possible issue regarding with the fix submitted in HEAD i.e. just looking at the code flow. Please, find below the comment at the fix and our proposal for the discussion. result is now proposed to be evaluated through the return value of StoreData; this return value is built with this logic below: 
result = DoRequest(myRequest,"<?php p($CFG->wwwroot) ?>/mod/scorm/datamodel.php","id=<?php p($id) ?>&sesskey=<?php p($USER->sesskey) ?>"+datastring);
        results = String(result).split('\n');
        errorCode = results[1];
        return results[0];
Everything will work as expected (i.e. true or false) until the AJAX call will fail: 
if (httpReq.status == 200) {
        //popupwin(url+"\n"+param+"\n"+httpReq.responseText);
        return httpReq.responseText;
    } else {
        return httpReq.status;
    }
so it could be IMHO better to code ''true if true, false otherwise'', e.g.: 
@@ -199,13 +199,20 @@
                         setTimeout('top.document.location=top.next;',500);
                     }
                 }
+                <?php
+                    if (debugging('',DEBUG_DEVELOPER)) {
+                        echo 'LogAPICall("LMSFinish", "AJAXResult", result, 0);';
+                    }
+                ?>
+                result = ('true' == result ? 'true' : 'false');
                 <?php
                     if (debugging('',DEBUG_DEVELOPER)) {
                         //echo 'alert("Finished SCORM 1.2");';
+                        echo 'LogAPICall("LMSFinish", "result", result, 0);';
                         echo 'LogAPICall("LMSFinish", param, "", 0);';
                     }
                 ?>
-                return "true";
+                return result;
             } else {
                 errorCode = "301";
             }
Note: I've rearranged also the LogAPICall parameters order, according to the signature&logic found in scorm_12.js.php. Note: we're interested in the fix of this issue also in 1.9 branch.
Hide
Permalink
Ron Meske added a comment -

I would like to express a sense of urgency for this. We have been experiencing this for the past few months where occasionaly a user would take a SCORM course and provide a screenprint of the final score showing completion, however there is absolutely no SCORM data, the status remains at Not Attempted. Based on less than 100 users, It is a small percentage of the total users at this time.

However, we are about to setup a new instlation of Moodle that will serve over 10,000 user starting in March and we are very concerned that the number of users experiencing this issue is going to climb rather quickly just by volume.

Show
Ron Meske added a comment - I would like to express a sense of urgency for this. We have been experiencing this for the past few months where occasionaly a user would take a SCORM course and provide a screenprint of the final score showing completion, however there is absolutely no SCORM data, the status remains at Not Attempted. Based on less than 100 users, It is a small percentage of the total users at this time. However, we are about to setup a new instlation of Moodle that will serve over 10,000 user starting in March and we are very concerned that the number of users experiencing this issue is going to climb rather quickly just by volume.
Hide
Permalink
Matteo Scaramuccia added a comment -

Let me add my 2 cents:

  1. Moodle should correctly implement the LMSFinish('') return value 'cause of a specification that tells how to do that. That's the reason for filing this bug as well as the sense of my review proposal;
  2. Content vendors should correctly react on a failure of an LMSFinish('') call (= having got "false" as the return value) by adopting a recovery strategy like e.g. try to re-call it after n seconds to mitigate e.g. network issues OR using sometime LMSCommit('').
    Those things above just to avoid false myths, see also Best practices for the use of Commit in SCORM content.

BTW, @Ron: if you're using Mastery Score in your packages, it could be possible that a failure in setting the cmi.core.lesson_status value could issue what described in MDL-21305.

Show
Matteo Scaramuccia added a comment - Let me add my 2 cents:
  1. Moodle should correctly implement the LMSFinish('') return value 'cause of a specification that tells how to do that. That's the reason for filing this bug as well as the sense of my review proposal;
  2. Content vendors should correctly react on a failure of an LMSFinish('') call (= having got "false" as the return value) by adopting a recovery strategy like e.g. try to re-call it after n seconds to mitigate e.g. network issues OR using sometime LMSCommit(''). Those things above just to avoid false myths, see also Best practices for the use of Commit in SCORM content.
BTW, @Ron: if you're using Mastery Score in your packages, it could be possible that a failure in setting the cmi.core.lesson_status value could issue what described in MDL-21305.
Hide
Permalink
Ron Meske added a comment -

I would like to point out the LMSCommit also needs this correction as it is also hardcoded to return true.

Show
Ron Meske added a comment - I would like to point out the LMSCommit also needs this correction as it is also hardcoded to return true.
Hide
Permalink
Ron Meske added a comment -

I also just looked at the scorm_13.js.php file and it also will need the corrections.

Show
Ron Meske added a comment - I also just looked at the scorm_13.js.php file and it also will need the corrections.
Hide
Permalink
Ron Meske added a comment -

After reviewing the flow of logic, the line: result = ('true' == result ? 'true' : 'false'); is not quite enough.

If it is set to false, then errorCode should also be set to conform to SCORM standards. I recommend the following:

result = ('true' == result) ? 'true' : 'false';
errorCode = (result == 'true')? '0' : '101';

Show
Ron Meske added a comment - After reviewing the flow of logic, the line: result = ('true' == result ? 'true' : 'false'); is not quite enough. If it is set to false, then errorCode should also be set to conform to SCORM standards. I recommend the following: result = ('true' == result) ? 'true' : 'false'; errorCode = (result == 'true')? '0' : '101';
Hide
Permalink
Ron Meske added a comment -

We have modified scorm_12_js.php based on all the input on this ticket and have done some limited testing. Our modified version is attached.

Show
Ron Meske added a comment - We have modified scorm_12_js.php based on all the input on this ticket and have done some limited testing. Our modified version is attached.
Hide
Permalink
Matteo Scaramuccia added a comment -

Good catch Ron!
I've missed the errorCode issue.

Thanks,
Matteo

Show
Matteo Scaramuccia added a comment - Good catch Ron! I've missed the errorCode issue. Thanks, Matteo
Hide
Permalink
Dan Marsden added a comment -

thanks to everyone who helped trace this - I've pushed the fix into 1.9Stable and HEAD - would appreciate it if someone sanity checked the commits!

thanks!

Show
Dan Marsden added a comment - thanks to everyone who helped trace this - I've pushed the fix into 1.9Stable and HEAD - would appreciate it if someone sanity checked the commits! thanks!

People

Vote (12)
Watch (8)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Moodle. Try JIRA - bug tracking software for your team.