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

Closed
temuri416 opened this Issue Dec 2, 2013 · 48 comments

Comments

Projects
None yet
4 participants
@temuri416
Contributor

temuri416 commented Dec 2, 2013

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!

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Dec 2, 2013

Could you please post a breif test case?

ghost commented Dec 2, 2013

Could you please post a breif test case?

@temuri416

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 Dec 2, 2013

Contributor

Do you have PHPEd?

Contributor

temuri416 commented Dec 2, 2013

Do you have PHPEd?

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Dec 2, 2013

No, but I think I can emulate script termination by using posix_kill(getmypid(), 9) in the catch block or exception handler.

ghost commented Dec 2, 2013

No, but I think I can emulate script termination by using posix_kill(getmypid(), 9) in the catch block or exception handler.

@temuri416

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 Dec 2, 2013

Contributor

ok, let me try to describe it in greater details.

  1. My app authenticates a user stores identity in session. I use file-based session adapter, session files are stored in /tmp/phpsess
  2. I throw an exception in a controller that requires presence of authenticated user.

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:

  1. From the browser I'm correctly getting "KABOOM" message and /tmp/phpsess/sess_en0f3prbiec3usp3060jq85ol3 file contains 'someValue' string.
  2. From the browser I click "Debug" button of PHPEd's toolbar. Execution interrupts at line 100, I step over it. When I step over Line 101, PHPEd is displaying an info message telling me that an Exception was raised.

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!

Contributor

temuri416 commented Dec 2, 2013

ok, let me try to describe it in greater details.

  1. My app authenticates a user stores identity in session. I use file-based session adapter, session files are stored in /tmp/phpsess
  2. I throw an exception in a controller that requires presence of authenticated user.

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:

  1. From the browser I'm correctly getting "KABOOM" message and /tmp/phpsess/sess_en0f3prbiec3usp3060jq85ol3 file contains 'someValue' string.
  2. From the browser I click "Debug" button of PHPEd's toolbar. Execution interrupts at line 100, I step over it. When I step over Line 101, PHPEd is displaying an info message telling me that an Exception was raised.

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!

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Dec 2, 2013

My test case:

<?php
session_start();
$_SESSION['a'] = 'b';
try {
        throw new Exception('xxx');
}
catch (Exception $e) {
        $pid = getmypid();
        `kill -15 {$pid}`;
}

kill -15 {$pid} emulates forceful termination of the process. In this case the session file is created but it remains empty.

If you set a breakpoint at this line: $pid = getmypid(); and then abort the debugging session, will the session file contain any data?

ghost commented Dec 2, 2013

My test case:

<?php
session_start();
$_SESSION['a'] = 'b';
try {
        throw new Exception('xxx');
}
catch (Exception $e) {
        $pid = getmypid();
        `kill -15 {$pid}`;
}

kill -15 {$pid} emulates forceful termination of the process. In this case the session file is created but it remains empty.

If you set a breakpoint at this line: $pid = getmypid(); and then abort the debugging session, will the session file contain any data?

@temuri416

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 Dec 2, 2013

Contributor

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 $pid = getmypid();. Initiating debugging from browser lands me on throw new Exception('xxx') line. If I reach getmypid(); inside the debugger - session gets saved OK, just as I described earlier.

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 /session_test.php in my browser and 'someValue' is equal to 'bbb' in my session file. All is well.

Next, I initiate debugging of session_test.php?a=123. Execution interrupts on throw new Exception('xxx');, debugging kicks in. Then, I terminate debugging session (without stepping further). Checking session file I see this:

someValue|

"bbb" set by first request was wiped out, and "bbb123" from the second was never stored.

Contributor

temuri416 commented Dec 2, 2013

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 $pid = getmypid();. Initiating debugging from browser lands me on throw new Exception('xxx') line. If I reach getmypid(); inside the debugger - session gets saved OK, just as I described earlier.

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 /session_test.php in my browser and 'someValue' is equal to 'bbb' in my session file. All is well.

Next, I initiate debugging of session_test.php?a=123. Execution interrupts on throw new Exception('xxx');, debugging kicks in. Then, I terminate debugging session (without stepping further). Checking session file I see this:

someValue|

"bbb" set by first request was wiped out, and "bbb123" from the second was never stored.

@temuri416

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 Dec 18, 2013

Contributor

@sjinks

What's your final thought on this?

Contributor

temuri416 commented Dec 18, 2013

@sjinks

What's your final thought on this?

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Dec 20, 2013

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.

ghost commented Dec 20, 2013

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.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Dec 20, 2013

Phalcon uses the same functions as the PHP code above and this is why I expect identical behavior in this case. Let's see…

ghost commented Dec 20, 2013

Phalcon uses the same functions as the PHP code above and this is why I expect identical behavior in this case. Let's see…

@temuri416

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 Dec 20, 2013

Contributor

Let me first confirm that PHPEd does not lose sessions when Phalcon is not enabled.

Contributor

temuri416 commented Dec 20, 2013

Let me first confirm that PHPEd does not lose sessions when Phalcon is not enabled.

@temuri416

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 Dec 20, 2013

Contributor

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.

Contributor

temuri416 commented Dec 20, 2013

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.

@temuri416

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 Dec 20, 2013

Contributor

With your session handler I am getting the original behaviour. Session is lost.

Contributor

temuri416 commented Dec 20, 2013

With your session handler I am getting the original behaviour. Session is lost.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Dec 20, 2013

Well, what is the difference between my session handler and the one you use in a non-Phalcon app?

ghost commented Dec 20, 2013

Well, what is the difference between my session handler and the one you use in a non-Phalcon app?

@temuri416

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 Dec 20, 2013

Contributor

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.

Contributor

temuri416 commented Dec 20, 2013

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.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Dec 20, 2013

This is what makes the difference: Zend_Session::writeClose();

ghost commented Dec 20, 2013

This is what makes the difference: Zend_Session::writeClose();

@temuri416

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 Dec 20, 2013

Contributor

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?

Contributor

temuri416 commented Dec 20, 2013

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?

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Dec 20, 2013

Yes, please

ghost commented Dec 20, 2013

Yes, please

@temuri416

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 Dec 20, 2013

Contributor

So I tested INVO.

Exactly the same problem.

The two logical conclusions:

  1. It's unrelated to PHPEd's DBG
  2. It's unrelated to whatever buggy code I may have written in my Phalcon app.
Contributor

temuri416 commented Dec 20, 2013

So I tested INVO.

Exactly the same problem.

The two logical conclusions:

  1. It's unrelated to PHPEd's DBG
  2. It's unrelated to whatever buggy code I may have written in my Phalcon app.
@temuri416

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 Dec 20, 2013

Contributor

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.

Contributor

temuri416 commented Dec 20, 2013

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.

@temuri416

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 Dec 20, 2013

Contributor

What is bothering me is the fact that you should step over at least one line of code while in debugger to preserve session.

Contributor

temuri416 commented Dec 20, 2013

What is bothering me is the fact that you should step over at least one line of code while in debugger to preserve session.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Dec 20, 2013

Could you please apply #1728 and see if this fixes the issue?

ghost commented Dec 20, 2013

Could you please apply #1728 and see if this fixes the issue?

@temuri416

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 Dec 20, 2013

Contributor

sure, only you have to give me the git command to pull that patch :-)

Contributor

temuri416 commented Dec 20, 2013

sure, only you have to give me the git command to pull that patch :-)

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Dec 21, 2013

OK, one sec, there is one failing test

ghost commented Dec 21, 2013

OK, one sec, there is one failing test

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Dec 21, 2013

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

ghost commented Dec 21, 2013

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
@temuri416

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 Dec 21, 2013

Contributor

crap, I did that and #1714 came back!

Let me comment that line out and try again.

Contributor

temuri416 commented Dec 21, 2013

crap, I did that and #1714 came back!

Let me comment that line out and try again.

@temuri416

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 Dec 21, 2013

Contributor

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.

Contributor

temuri416 commented Dec 21, 2013

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.

@temuri416

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 Dec 21, 2013

Contributor

hm... found this:

if (!nusphere_dbg_present) {
    phalcon_di_object_handlers.get_properties = phalcon_di_get_properties;
}

strange. still, let me comment it out.

Contributor

temuri416 commented Dec 21, 2013

hm... found this:

if (!nusphere_dbg_present) {
    phalcon_di_object_handlers.get_properties = phalcon_di_get_properties;
}

strange. still, let me comment it out.

@temuri416

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 Dec 21, 2013

Contributor
  1. commenting out phalcon_di_object_handlers.get_properties = phalcon_di_get_properties; stopped crashing PHPEd. looks like if (!nusphere_dbg_present) { test isn't working - DBG is on for certain.
  2. with your patch things got even worse. throwing exception wipes out user session even when PHPEd debugging is not running (or, for that matter, when PHPEd itself isn't running).

just FYI - I'm now in a different environment from the one when we were working on #1714

Contributor

temuri416 commented Dec 21, 2013

  1. commenting out phalcon_di_object_handlers.get_properties = phalcon_di_get_properties; stopped crashing PHPEd. looks like if (!nusphere_dbg_present) { test isn't working - DBG is on for certain.
  2. with your patch things got even worse. throwing exception wipes out user session even when PHPEd debugging is not running (or, for that matter, when PHPEd itself isn't running).

just FYI - I'm now in a different environment from the one when we were working on #1714

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Dec 21, 2013

OK and if you add

public function __destruct()
{
    session_write_close();
}

to PhalconSessionAdapter?

Regarding #1714: dbg-php is still shown as DBG in php -m and is loaded before Phalcon?

ghost commented Dec 21, 2013

OK and if you add

public function __destruct()
{
    session_write_close();
}

to PhalconSessionAdapter?

Regarding #1714: dbg-php is still shown as DBG in php -m and is loaded before Phalcon?

@temuri416

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 Dec 21, 2013

Contributor
  1. yes, it is still "DBG":
[Zend Modules]
DBG
  1. I was wrong about "things got even worse". Nothing has changed, and exceptions outside PHPEd debugger do not cause session loss.
  2. THAT FIXED IT! I can terminate debugging without stepping further and session does get saved. HURRAY!

So, what does it all mean?

Contributor

temuri416 commented Dec 21, 2013

  1. yes, it is still "DBG":
[Zend Modules]
DBG
  1. I was wrong about "things got even worse". Nothing has changed, and exceptions outside PHPEd debugger do not cause session loss.
  2. THAT FIXED IT! I can terminate debugging without stepping further and session does get saved. HURRAY!

So, what does it all mean?

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Dec 21, 2013

THAT FIXED IT!

I suspected that :-) session_write_close() forces all session data to be flushed and then closes the session. Flushing ensures that all data are written to the disk or whatever the session storage is.

Without session_write_close() there could be partial writes — this is exactly what we saw.

I guess this issue is fixed, but #1714 still remains, correct?

ghost commented Dec 21, 2013

THAT FIXED IT!

I suspected that :-) session_write_close() forces all session data to be flushed and then closes the session. Flushing ensures that all data are written to the disk or whatever the session storage is.

Without session_write_close() there could be partial writes — this is exactly what we saw.

I guess this issue is fixed, but #1714 still remains, correct?

@temuri416

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 Dec 21, 2013

Contributor
  1. so are you saying that I should use custom session handler in my Phalcon app now? what I had before was this:
$config->merge(new Phalcon\Config([
    'services' => [
        'session' => [
            'className' => 'Phalcon\Session\Adapter\Files',
            'calls' => [
                [
                    'method' => 'start'
                ]
            ],
        ],

Do I still need your 1624 patch?

  1. #1714 is a bit unclear. I recall pulling the code immediately after your PR was merged in, and it was working fine. However it all went to crap again when I pulled https://github.com/sjinks/cphalcon issue-1624.
Contributor

temuri416 commented Dec 21, 2013

  1. so are you saying that I should use custom session handler in my Phalcon app now? what I had before was this:
$config->merge(new Phalcon\Config([
    'services' => [
        'session' => [
            'className' => 'Phalcon\Session\Adapter\Files',
            'calls' => [
                [
                    'method' => 'start'
                ]
            ],
        ],

Do I still need your 1624 patch?

  1. #1714 is a bit unclear. I recall pulling the code immediately after your PR was merged in, and it was working fine. However it all went to crap again when I pulled https://github.com/sjinks/cphalcon issue-1624.
@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Dec 21, 2013

Now I am confused: what exactly worked: public function __destruct() or the PR from issue-1624 branch?

ghost commented Dec 21, 2013

Now I am confused: what exactly worked: public function __destruct() or the PR from issue-1624 branch?

@temuri416

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 Dec 21, 2013

Contributor

oh. I added public function __destruct() while being on issue-1624 branch.

should I go back to main branch?

Contributor

temuri416 commented Dec 21, 2013

oh. I added public function __destruct() while being on issue-1624 branch.

should I go back to main branch?

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Dec 21, 2013

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.

ghost commented Dec 21, 2013

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.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Dec 21, 2013

My last commit should fix #1714 as well.

ghost commented Dec 21, 2013

My last commit should fix #1714 as well.

@temuri416

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 Dec 21, 2013

Contributor

ok, finally:

  1. did what you told me, threw out PhalconSessionAdapter and all is well. session is not lost if debugger is killed
  2. #1714 looks to be fixed now.

very happy now. looking forward to merge!

thanks a lot!

Contributor

temuri416 commented Dec 21, 2013

ok, finally:

  1. did what you told me, threw out PhalconSessionAdapter and all is well. session is not lost if debugger is killed
  2. #1714 looks to be fixed now.

very happy now. looking forward to merge!

thanks a lot!

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Dec 21, 2013

Always welcome :-)

ghost commented Dec 21, 2013

Always welcome :-)

@roman-kulish

This comment has been minimized.

Show comment
Hide comment
@roman-kulish

roman-kulish Jan 13, 2014

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.

roman-kulish commented Jan 13, 2014

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.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jan 13, 2014

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.

ghost commented Jan 13, 2014

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.

@roman-kulish

This comment has been minimized.

Show comment
Hide comment
@roman-kulish

roman-kulish Jan 13, 2014

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 :)

roman-kulish commented Jan 13, 2014

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 :)

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jan 14, 2014

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

ghost commented Jan 14, 2014

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

@temuri416

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 Mar 1, 2014

Contributor

@sjinks

Hi Vladimir,

Unfortunately, this issue is back, exactly as I had originally described it.

The commit that introduced it is d5fcf03.

Perhaps you could have another look at it.

Thank you.

Contributor

temuri416 commented Mar 1, 2014

@sjinks

Hi Vladimir,

Unfortunately, this issue is back, exactly as I had originally described it.

The commit that introduced it is d5fcf03.

Perhaps you could have another look at it.

Thank you.

@temuri416

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 May 9, 2014

Contributor

No longer an issue.

Contributor

temuri416 commented May 9, 2014

No longer an issue.

@temuri416 temuri416 closed this May 9, 2014

@temuri416

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 Aug 23, 2015

Contributor

@andresgutierrez

Andres,

It looks like this issue has resurfaced in Phalcon 2.

Original description of the problem:

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.

These commits show what @sjinks did to fix it on 1.3 branch:

64a2f60
41da044

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!

Contributor

temuri416 commented Aug 23, 2015

@andresgutierrez

Andres,

It looks like this issue has resurfaced in Phalcon 2.

Original description of the problem:

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.

These commits show what @sjinks did to fix it on 1.3 branch:

64a2f60
41da044

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!

@temuri416 temuri416 reopened this Aug 23, 2015

andresgutierrez added a commit that referenced this issue Aug 23, 2015

@andresgutierrez

This comment has been minimized.

Show comment
Hide comment
@andresgutierrez

andresgutierrez Aug 23, 2015

Member

Fixed in the 2.0.x branch

Member

andresgutierrez commented Aug 23, 2015

Fixed in the 2.0.x branch

@temuri416

This comment has been minimized.

Show comment
Hide comment
@temuri416

temuri416 Aug 24, 2015

Contributor

@andresgutierrez

Confirmed! Thanks a lot.

Contributor

temuri416 commented Aug 24, 2015

@andresgutierrez

Confirmed! Thanks a lot.

patrick-zippenfenig pushed a commit to patrick-zippenfenig/cphalcon that referenced this issue Aug 26, 2015

r3jack added a commit to r3jack/cphalcon that referenced this issue Sep 21, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment