-
-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BUG ... possibly?] Exceptions cause loss of user session when remote debugging is on. #1624
Comments
Could you please post a breif test case? |
Do you have PHPEd? |
No, but I think I can emulate script termination by using |
ok, let me try to describe it in greater details.
All exceptions are handled and user is shown a generic "KABOOM" message. Exception handling code: $eventsManager->attach('dispatch:beforeException', function($event, $dispatcher, $exception) {
$message = $exception->getMessage() . ' (' . $exception->getFile() . ':' . $exception->getLine() . ')';
if ($dispatcher->getDI()->has(LOGD_EXCEPTION)) {
$dispatcher->getDI()->get(LOGD_EXCEPTION)->error($message);
} else {
error_log($message);
}
if ($dispatcher->getDI()->getRequest()->isAjax()) {
$dispatcher->getDI()->getResponse()->setStatusCode(400, 'Application error.');
} else {
$dispatcher->forward([
'controller' => 'error',
'action' => 'index',
]);
}
return false;
}); The relevant code from my Controller's action, what happens elsewhere isn't really important. Line 100: $_SESSION['someKey'] = 'someValue'; // PHPEd's breakpoint is set on this line
Line 101: throw new Exception('Just because'); Of course, line 100 above is there for illustration/debugging purposes. Here are the cases:
At that moment I have two options - I can let execution continue in the debugger (and session will be preserved and all will be fine) OR I can break and execution will remain at line 101. Now comes the important part. If I decide to break AND step over any following line at least once - session is preserved and /tmp/phpsess/sess_en0f3prbiec3usp3060jq85ol3 file will contain 'someValue' string, among other stuff that I have in session at that moment. HOWEVER, if, having halted post-exception execution, I terminate debugging altogether, without stepping over any single line - session is lost. What's interesting, my /tmp/phpsess/sess_en0f3prbiec3usp3060jq85ol3 file will contain the following string only: MyApp\AccessControl\Security|MyApp\Controllers\LoginController|$PMM$|MYAPP_USER_AUTH_NAMESPACE|someKey| Somehow, I don't believe that PHPEd has anything to do with session corruption. It's not like client's losing session cookie value. As you can see - nothing gets written. I'm inclined to think that something goes wrong in Phalcon's session handler. Anything else I can do to help? Thanks! |
My test case: <?php
session_start();
$_SESSION['a'] = 'b';
try {
throw new Exception('xxx');
}
catch (Exception $e) {
$pid = getmypid();
`kill -15 {$pid}`;
}
If you set a breakpoint at this line: |
a). Executing your code "as-is" in the browser gives me nginx error: An error occurred.
Sorry, the page you are looking for is currently unavailable.
Please try again later.
If you are the system administrator of this resource then you should check the error log for details.
Faithfully yours, nginx. b). With your code I cannot trap execution at c). I've modified your code slightly for a different test: <?php
session_start();
$a = isset($_GET['a']) ? $_GET['a'] : null;
$_SESSION['someValue'] = 'bbb' . $a;
if ($a) {
try {
throw new Exception('xxx');
}
catch (Exception $e) {
$pid = getmypid();
`kill -15 {$pid}`;
}
} First, I open Next, I initiate debugging of someValue| "bbb" set by first request was wiped out, and "bbb123" from the second was never stored. |
What's your final thought on this? |
Let's try to replace Phalcon's session with the native session. This is the PHP's equivalent for Phalcon\Session\Adapter (Phalcon\Session\Adapter\Files just inherits from Phalcon\Session\Adapter and does not introduce any functionality at all): <?php
class PhalconSessionAdapter implements \Phalcon\Session\AdapterInterface
{
protected $_uniqueId = '';
protected $_started = false;
protected $_options = array();
public function __construct($options = null)
{
if (is_array($options)) {
$this->setOptions($options);
}
}
public function start()
{
if (!headers_sent()) {
session_start();
$this->_started = true;
return true;
}
return false;
}
public function setOptions($options)
{
if (is_array($options)) {
if (isset($options['uniqueId'])) {
$this->_uniqueId = $options['uniqueId'];
}
$this->_options = $options;
}
}
public function getOptions()
{
return $this->_options;
}
public function get($index, $default = null, $remove = false)
{
$key = $this->_uniqueId . $index;
if (isset($_SESSION[$key])) {
$result = $_SESSION[$key];
if ($remove) {
unset($_SESSION[$key]);
}
if (!empty($result)) {
return $result;
}
}
return $default;
}
public function set($index, $value)
{
$key = $this->_uniqueId . $index;
$_SESSION[$key] = $value;
}
public function has($index)
{
$key = $this->_uniqueId . $index;
return isset($_SESSION[$key]);
}
public function remove($index)
{
$key = $this->_uniqueId . $index;
unset($_SESSION[$key]);
}
public function getId()
{
return session_id();
}
public function isStarted()
{
return $this->_started;
}
public function destroy()
{
$this->_started = false;
session_destroy();
return true;
}
} Just do smth like this in your code when you are setting Phalcon's session: $di['session'] = function() { $s = new PhalconSessionAdapter(); $s->start(); return $s; }; and then see if PhpEd is able to work with this replacement. |
Phalcon uses the same functions as the PHP code above and this is why I expect identical behavior in this case. Let's see… |
Let me first confirm that PHPEd does not lose sessions when Phalcon is not enabled. |
so, I have another app that does not require Phalcon and exceptions caught by PHPEd do not cause session loss. let me now run your code. |
With your session handler I am getting the original behaviour. Session is lost. |
Well, what is the difference between my session handler and the one you use in a non-Phalcon app? |
Here's my non-Phalcon session handler: <?php
namespace App\Session;
use Zend_Session_SaveHandler_Interface,
Zend_Cache_Core,
Zend_Session;
class SaveHandler implements Zend_Session_SaveHandler_Interface
{
/**
* Zend Cache instance
*
* @var Zend_Cache
*/
protected $cache;
/**
* Sets Zend_Cache instance.
*
* @param Zend_Cache_Core $cache Zend_Cache instance
*/
public function __construct(Zend_Cache_Core $cache)
{
$this->cache = $cache;
}
/**
* Destructor.
*
* @return void
*/
public function __destruct()
{
Zend_Session::writeClose();
}
/**
* Open session.
*
* @param string $save_path
* @param string $name
* @return boolean
*/
public function open($save_path, $name)
{
return true;
}
/**
* Close session.
*
* @return boolean
*/
public function close()
{
return true;
}
/**
* Read session data.
*
* @param string $id Memcache key name
* @return string
*/
public function read($id)
{
if (!$data = $this->cache->load($id)) {
return null;
}
return $data;
}
/**
* Write session data.
*
* @param string $id Memcache key name
* @param string $sessionData Serialized data
* @return boolean
*/
public function write($id, $sessionData)
{
$lifetime = (int) $this->cache->getOption('lifetime');
$return = $this->cache->save(
$sessionData,
$id,
array(),
$lifetime);
return $return;
}
/**
* Destroy session.
*
* @param string $id Memcache key name
* @return boolean
*/
public function destroy($id)
{
return $this->cache->remove($id);
}
/**
* Garbage collection.
*
* @param int $maxlifetime
* @return true
*/
public function gc($maxlifetime)
{
return true;
}
} It is using memcached as backend. Actually, I am gonna change the backend to filesystem there. Let's see. |
This is what makes the difference: |
File-based backend in non-Phalcon app is OK. Session is not lost. I can take INVO sample app and see how it behaves. Should I do that? |
Yes, please |
So I tested INVO. Exactly the same problem. The two logical conclusions:
|
And, I do not think it's client losing session cookie. My previous experiment showed that session DOES get written on shutdown, but it is truncated. |
What is bothering me is the fact that you should step over at least one line of code while in debugger to preserve session. |
Could you please apply #1728 and see if this fixes the issue? |
sure, only you have to give me the git command to pull that patch :-) |
OK, one sec, there is one failing test |
The command is git checkout 1.3.0
git checkout -b test-1624 # This will create a new branch from 1.3.0, you can delete it when done
git pull https://github.com/sjinks/cphalcon issue-1624 |
crap, I did that and #1714 came back! Let me comment that line out and try again. |
hm, looks like you did change the code in ext/di.c - line of code from #1714 fix isn't there anymore. PHPEd is crashing now. |
hm... found this: if (!nusphere_dbg_present) {
phalcon_di_object_handlers.get_properties = phalcon_di_get_properties;
} strange. still, let me comment it out. |
just FYI - I'm now in a different environment from the one when we were working on #1714 |
$config->merge(new Phalcon\Config([
'services' => [
'session' => [
'className' => 'Phalcon\Session\Adapter\Files',
'calls' => [
[
'method' => 'start'
]
],
], Do I still need your 1624 patch?
|
Now I am confused: what exactly worked: |
oh. I added should I go back to main branch? |
Just kill this branch: git checkout 1.3.0
git branch -D test-1624 Then update the repo: git fetch --all
git merge upstream/1.3.0 And re-fetch my changes (I have pushed one more commit): git checkout -b test-1624
git pull https://github.com/sjinks/cphalcon issue-1624 Then build Phalcon and test your application without PhalconSessionAdapter — I hope it should work without session loss. |
My last commit should fix #1714 as well. |
ok, finally:
very happy now. looking forward to merge! thanks a lot! |
Always welcome :-) |
Is it safe? For example, Adapter::__destruct() calls session_write_close() that calls registered earlier session handler method; session handler attempts to use a database object to flush session data -- at this point the database object may have been already destroyed. |
If the session handler has a reference to the database object, the database object will still be valid. Objects are destroyed only after the last reference to them is lost. |
It does not include weak references and use cases such as factories, for examples: a developed passes DI container to adapter's constructor. Can you be sure that a database object will exist and will be successfully returned by DI? Consider internal DB object dependencies as well. I'm still trying to convince the team to provide normal interface for sessions with proper close () and GC() method :) |
Why don't you just add 'implements \SessionHandlerInterface' when you really need it? Your proposed change will break the existing code, ie https://github.com/phalcon/incubator/blob/master/Library/Phalcon/Session/Adapter/Redis.php |
No longer an issue. |
Andres, It looks like this issue has resurfaced in Phalcon 2. Original description of the problem:
These commits show what @sjinks did to fix it on 1.3 branch: I looked at https://github.com/phalcon/cphalcon/blob/master/phalcon/session/adapter.zep and it indeed does not have a destructor. Could you please take a look? Thanks! |
Fixed in the 2.0.x branch |
Confirmed! Thanks a lot. |
OK, I might be completely wrong here. But, you never know - maybe I stumbled upon some obscure real issue - so here it comes.
I use PHPEd remote debugger when developing. Every time when my application throws an exception and it is caught by PHPEd, AND I decide to terminate script execution, I lose user session.
If I let the execution continue, session is preserved.
Session is not lost when debugger is off and it does not matter if exception is left unhandled.
I have never seen anything similar in all my years dealing with remote debugging. So, my question is - could it be that Phalcon's session handler "forgets" the session variables when exception is thrown and execution is aborted from within the IDE?
I am 100% sure that PHPEd does not flush any superglobals - I've been debugging Exceptions for a long time and never experienced anything similar.
I can reliably reproduce this behaviour, if anyone from development team wants to have a closer look.
Thanks!
The text was updated successfully, but these errors were encountered: