1
\$\begingroup\$
function executeQuery() {
  $.ajax({
    url: '../include/activity.php',
    success: function(data) {
      // do something with the return value here if you like
      if(data.error == true){
           window.location.href = "login.php";
      }

    }
  });
  setTimeout(executeQuery, 3000); // you could choose not to continue on failure...
}

$(document).ready(function() {
  // run the first time; all subsequent calls will take care of themselves
  setTimeout(executeQuery, 3000);
});

In activity.php

if (isset($_SESSION['LAST_ACTIVITY']) && (time() - $_SESSION['LAST_ACTIVITY'] > 300)) {
    // last request was more than 5 minutes ago
    session_unset();     // unset $_SESSION variable for the run-time
    session_destroy();   // destroy session data in storage
echo json_encode(array(
    'error' => true
));    
}
$_SESSION['LAST_ACTIVITY'] = time(); // update last activity time stamp


if (!isset($_SESSION['CREATED'])) {
    $_SESSION['CREATED'] = time();
    echo json_encode(array(
        'error' => false
    ));    
} else if (time() - $_SESSION['CREATED'] > 300) {
    // session started more than 5 minutes ago
    session_regenerate_id(true);    // change session ID for the current session an invalidate old session ID
    $_SESSION['CREATED'] = time();  // update creation time
    echo json_encode(array(
        'error' => false
    ));    
}

This is how I am checking for activity of user using ajax and php. I have doubts if this is acceptable in real life environment. I would like to have suggestion on how to implement this cleanly , correctly easily.

\$\endgroup\$
6
  • \$\begingroup\$ If your javascript code work only for logged users, than you have 20 request per minute for every logged user, and then the normal user activity requests. So is just a matter of how many users do you have, and how many of those users will connect on the same time. \$\endgroup\$ Sep 7 '16 at 9:14
  • \$\begingroup\$ On executeQuery() I'll move the setTimeout() staff inside the handler of ajax call, so ti will start a new requst only if is back from the previous. So if your server slow down for some reason (could not answer in less than 3 seconds) you're going to slow down the requests too. \$\endgroup\$ Sep 7 '16 at 9:16
  • \$\begingroup\$ you have 20 request per minute please clarify this one. I dont have exact number of user it could be more \$\endgroup\$
    – guradio
    Sep 7 '16 at 9:16
  • \$\begingroup\$ you scheduled a request every 3 seconds (3000 milliseconds), so this results in 20 requests per minute. \$\endgroup\$ Sep 7 '16 at 9:18
  • \$\begingroup\$ @MarioAlexandroSantini what is the best option for me to tackle into? \$\endgroup\$
    – guradio
    Sep 7 '16 at 9:23
2
\$\begingroup\$

I would route all ajax calls through 1 function in JavaScript, and that function then keeps track of the last call. In essence, the browser itself should know whether there was activity or not.

If for some odd reason you need multiple windows with the same session to stay open as long as one of them has activity, then I would change the timer from 3000 to 60000. Checking once a minute should be more than fine.

Finally executeQuery is a terrible name, perhaps checkTimeOut?

\$\endgroup\$
0
\$\begingroup\$

I don't understand how the API can ever return error = true in a valid manner. Each time that your last activity timeout is reached, you unset $_SESSION, so the first conditional below would always execute, meaning you have now echoed two JSON fragments to the caller (invalid JSON when considered as a whole).

More context-specific comments are in the code below within multi-line comments.

Javascript

/*
Agree with other answer that this is named poorly.
Perhaps activityPoller() or something more meangingful.
*/
function executeQuery() {
/*
This only considers happy path of 200 response from server.  You should
handle other potential server responses as well.
*/
/*
  $.ajax({
    url: '../include/activity.php',
    success: function(data) {
      // do something with the return value here if you like
/*
Use exact comparison here to make your code less fragile to unexpected
"truthy" responses.
*/
      if(data.error == true){
           window.location.href = "login.php";
      }

    }
  });
/*
Consider attaching .done() to your ajax call and triggering timeout reset
from that. That would have you setting the timeouts after the ajax call has
completed as opposed to basically instantaneously after the ajax call is
initiated.
*/
  setTimeout(executeQuery, 3000); // you could choose not to continue on failure...
}

$(document).ready(function() {
  // run the first time; all subsequent calls will take care of themselves
  setTimeout(executeQuery, 3000);
});

PHP

/*
Consider breaking out your enforced session length into a constant rather
than have it first appear as an undocumented integer value inside a conditional.
define('SESSION_LENGTH', 300);
/*
/*
Consider replacing multiple calls to time() in this code to a single variable.
That way you don't get behavior where last activity and start get out of
sync by 1 second around time boundary conditions.
Just set $time variable or similar right here and use that reference throughout.
*/
/*
This line of code is a bit long and hard to read. Consider splitting the
conditional across lines.
Consider using:
!empty($_SESSION['LAST_ACTIVITY'])
as it is more complete as a guarding condition.
*/
if (isset($_SESSION['LAST_ACTIVITY']) 
if (isset($_SESSION['LAST_ACTIVITY']) && (time() - $_SESSION['LAST_ACTIVITY'] > 300)) {
/*
Comment not needed if time limit is defined in configuration as suggested.
Let the code speak for itself.
*/
    // last request was more than 5 minutes ago
/*
Generally, don't put comments at the end of the code lines. It makes your
comments harder to read. Place comments in line immediately
above relevant section of code. Typical throughout. 
*/
    session_unset();     // unset $_SESSION variable for the run-time
    session_destroy();   // destroy session data in storage
/*
Are you using session cookies? Do you need to destroy the cookie as well?
*/
/*
Defer output to the end. Perhaps just set $error variable with boolean value
or build an actual $response array that you populate in your code to then be
serialized and output.
*/
echo json_encode(array(
    'error' => true
));    
}

$_SESSION['LAST_ACTIVITY'] = time(); // update last activity time stamp

/*
Use empty(...) instead of !isset(...) here.
You can easily get rid of this if-else condition
*/
if (!isset($_SESSION['CREATED'])) {
    $_SESSION['CREATED'] = time();
    echo json_encode(array(
        'error' => false
    )); 
/*
Again use constant here for session length limit.
Or, if you think you will ever need to independently control the
last activity timeout independently from the session id rotation
timeout this might be an additional constant.
*/
} else if (time() - $_SESSION['CREATED'] > 300) {
    // session started more than 5 minutes ago
    session_regenerate_id(true);    // change session ID for the current session an invalidate old session ID
    $_SESSION['CREATED'] = time();  // update creation time
    echo json_encode(array(
        'error' => false
    ));    
}

/*
Output JSON here.
*/
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.