Skip to content
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

Reconect to database if connect closed #12634

Closed
Mirocow opened this issue Sep 29, 2016 · 56 comments
Closed

Reconect to database if connect closed #12634

Mirocow opened this issue Sep 29, 2016 · 56 comments

Comments

@Mirocow
Copy link

Mirocow commented Sep 29, 2016

please refactor class \yii\db\Command

        $token = $rawSql;
        try {
            Yii::beginProfile($token, 'yii\db\Command::query');

            if(!$this->isConnected()){
                $this->db->close();
                $this->db->open();
            }

            $this->pdoStatement->execute();

            if ($method === '') {
                $result = new DataReader($this);
            } else {
                if ($fetchMode === null) {
                    $fetchMode = $this->fetchMode;
                }
                $result = call_user_func_array([$this->pdoStatement, $method], (array) $fetchMode);
                $this->pdoStatement->closeCursor();
            }

            Yii::endProfile($token, 'yii\db\Command::query');
        } catch (\Exception $e) {
            Yii::endProfile($token, 'yii\db\Command::query');
            throw $this->db->getSchema()->convertException($e, $rawSql);
        }
    public function isConnected()
    {
        // Some check code 
        return false;
    }
@samdark samdark added this to the 2.0.11 milestone Sep 29, 2016
@samdark
Copy link
Member

samdark commented Sep 29, 2016

I think it's nice to have feature.

@solovjov
Copy link

@anatoliyarkhipov
Copy link

It may be useful when you write a service or a daemon

@pernatiy02
Copy link

pernatiy02 commented Sep 29, 2016

add please! I need it too!

@samdark
Copy link
Member

samdark commented Sep 29, 2016

@solovjov, @anatoliyarkhipov, @pernatiy02 what do you think about the solution @Mirocow proposed?

@klimov-paul
Copy link
Member

Constant triggering of SQL statement even such simple as SELECT 1 will degrade performance and produce extra workload on the SQL server side.

@samdark
Copy link
Member

samdark commented Sep 29, 2016

For MySQL it could be DO 1 which should affect performance less. Not sure it would work with other DBs though.

@klimov-paul could it be avoided while achieving reconnecting?

@klimov-paul
Copy link
Member

klimov-paul commented Sep 29, 2016

The only possible solution is wrapping existing execute() in try...catch and handle closed by timout connection via exception code like it is done at yii\db\IntegrityException.

public function execute()
{
    try {
       return $this->executeInternal();
    } catch (Exception $e) {
        if (/* $e indicates DB connection is closed by timeout */) {
            $this->db->close();
            $this->db->open();
            return $this->executeInternal();
        }
        throw $e;
    }
}

@Mirocow
Copy link
Author

Mirocow commented Sep 29, 2016

We may add flag permanent such

        'db' => [
            'class' => 'yii\db\Connection',
            'dsn' => 'mysql:host=localhost;dbname=mydb',
            'username' => 'root',
            'password' => 'password',
            'charset' => 'utf8',
            'tablePrefix' => 'tbl_',
            // turn on schema caching to improve performance
            'schemaCache' => 'db_cache',
            'schemaCacheDuration' => 3600,
            'enableSchemaCache' => false,
            'permanent': true,
        ],

@Mirocow
Copy link
Author

Mirocow commented Sep 29, 2016

DO 1; very nice sql query

@Mirocow
Copy link
Author

Mirocow commented Sep 29, 2016

public function execute()
{
    try {
       return $this->executeInternal();
    } catch (Exception $e) {
        if (/* $e indicates DB connection is closed by timeout */) {
            $this->db->close();
            $this->db->open();
            return $this->executeInternal();
        }
        throw $e;
    }
}

bad idea because sql query should be very hard

@Shkarbatov
Copy link

This is terrible! Please don't do this!

@SilverFire
Copy link
Member

SilverFire commented Sep 29, 2016

Duplicates #10168
Discussion on forum for Yii 1: http://www.yiiframework.com/forum/index.php/topic/20063-general-error-2006-mysql-server-has-gone-away/

@Mirocow
Copy link
Author

Mirocow commented Sep 29, 2016

This is terrible! Please don't do this!

why?
We may add flag permanent #12634 (comment)

@SilverFire SilverFire changed the title Please add reconect to database if connect closed Reconect to database if connect closed Sep 29, 2016
@Shkarbatov
Copy link

Shkarbatov commented Sep 29, 2016

As for me, better to do like this:

    <?php
    namespace app\components\db;
    use yii\db\Exception;

    class Command extends \yii\db\Command
    {
        /**
         * @var ReConnection
         */
        public $db;

        protected function queryInternal($method, $fetchMode = null)
        {
            try {
                return parent::queryInternal($method, $fetchMode);

            } catch (Exception $e) {
                $this->cancel();
                $this->db->reconnect();

                $this->bindValues($this->params);

                return parent::queryInternal($method, $fetchMode);
            }
        }

    }
    <?php
    namespace app\components\db;

    use Yii;
    use yii\base\Exception;
    use yii\db\Connection;
    use app\components\db\mysql\PDO;

    class ReConnection extends Connection
    {
        /** @var int - attempts retry */
        public $retry_attempts = 10;

        /** @var int - millisecond sleep time */
        public $sleep_time = 100;

        /** @var string */
        public $commandClass = Command::class;

        /**
         * Creates a command for execution.
         * @param string $sql the SQL statement to be executed
         * @param array $params the parameters to be bound to the SQL statement
         * @return Command the DB command
         */
        public function createCommand($sql = null, $params = [])
        {
            $class_name = $this->commandClass
            $command = new $class_name([
                'db' => $this,
                'sql' => $sql,
            ]);

            return $command->bindValues($params);
        }


        /**
         * ReCreates the PDO instance.
         *
         * @return PDO the pdo instance
         * @throws Exception - Cannot connect to db
         */
        protected function createPdoInstance()
        {
            $count = 0;

            do {
                try {
                    return parent::createPdoInstance();

                } catch (\PDOException $e) {
                    if (++$count > $this->retry_attempts) {
                        break;
                    }
                    usleep($this->sleep_time);

                    Yii::info("Reconnect attempt: $count ". $this->dsn, __METHOD__);
                }
            } while (true);

            throw $e;
        }

        /**
         * @throws \yii\base\InvalidConfigException
         * @throws \yii\db\Exception
         */
        public function reconnect()
        {
            Yii::info('Reopening DB connection: ' . $this->dsn, __METHOD__);

            $this->close();
            $this->open();
        }
    }

@Mirocow
Copy link
Author

Mirocow commented Sep 29, 2016

so hard for me ;)

@Shkarbatov
Copy link

Are you trying to check you code?

$this->_instance

There no variable _instance in Command class

if(!$this->isConnected()){
    \Yii::$app->db->close();
    \Yii::$app->db->open();
}

$this->pdoStatement->execute();

Variable pdoStatement will content old instance of PDO, not a new one.

@Mirocow
Copy link
Author

Mirocow commented Sep 29, 2016

fixed

            if(!$this->isConnected()){
                $this->db->close();
                 $this->db->open();
            }

because this code should be in the namespace \yii\db\Command

@Shkarbatov
Copy link

There no variable _instance in \yii\db\Command

@AnatolyRugalev
Copy link
Contributor

AnatolyRugalev commented Sep 29, 2016

Faced this problem in my long-running queue workers. Found that the best solution is to store date of open operation and check duration of connection. I introduced $reconnectTimeout in Connection which determines when to reconnect.

Something like that:

if (time() - $this->_openedAt > $this->reconnectTimeout) {
    $this->close();
}

@SilverFire
Copy link
Member

@Mirocow I don't know why you've reacted with thumb down on comment on my previous comment if you think it's not related - explain your opinion, please.

I see the following use cases and solutions for the problem:

  1. Long action in common application. Solution: increase wait_timeout as it was described in the issue I've linked as duplication.
  2. Daemon application that can sleep for a long time. Solution: try-catch in execute() method, as @klimov-paul proposed in his comment or extend Connection and register a customized one as a component.

Why do I think auto-reconnect can not be accepted:

  • See the use case: I start a transaction, do some DB-work, then a long PHP operation, then DB again. During the long PHP operation, connection was gone and in case we have no auto-reconnect, we'll get an exception. Otherwise, we'll have inconsistent data because the previous transaction was reverted because of connection timeout and the new one will just COMMIT new changes.
  • Usually we do many SQL queries during a short amount of time. Do you really think it's a good idea to multiply SQL queries count by 2 just to know, that the connection is alive after a previous query, that was sent probable less than 20ms ago? (Think about a timer)

@Mirocow
Copy link
Author

Mirocow commented Sep 29, 2016

because we must execute very simle sql query

in my app i did such

        // TODO 6: Пофиксить когда реконект запилят в yii2
        try{
            \Yii::$app->db->createCommand("DO 1")->execute();
        }catch (\yii\db\Exception $e){
            \Yii::$app->db->close();
            \Yii::$app->db->open();
        }

        $sheet = PriceList::findOne($message['id']);

@AnatolyRugalev
Copy link
Contributor

AnatolyRugalev commented Sep 29, 2016

Another way is to introduce idle event in Yii application. Developer can call it when script is going idle.

Yii::$app->trigger('idle');

This will allow us to close any kind of resources in framework core.

// Any connection based component
public function init()
{
    Yii::$app->on('idle', [$this, 'close']);
    parent::init()
}

This may be useful only in daemon scripts, but I think you should consider this solution for its flexibility.

@SilverFire
Copy link
Member

SilverFire commented Sep 29, 2016

Daemons is a complex thing and daemons know a plenty of ways how to fuck you up :)
Gone SQL connection is probably one of the easiest things you should worry about.

There are much wiser ways to handle gone connection than sending SQL pings through it each time you want to execute something real. And @AnatolyRugalev have posted some of them.

I think we should close the issue.

@Mirocow do you have anything to say about the transactions use case I've posted?

@Mirocow
Copy link
Author

Mirocow commented Sep 29, 2016

@SilverFire another case, we write daemon or service and our service execute some queries for some settings or update some flags

@Shkarbatov
Copy link

Shkarbatov commented Sep 29, 2016

    if (time() - $this->_openedAt > $this->reconnectTimeout) {
        $this->close();
    }

👍

Motorized crutch
Моторизированный костыль

But if you will have a network problem it will not help :)

@Mirocow
Copy link
Author

Mirocow commented Sep 29, 2016

http://www.php.net/pdo.connections

<?php
$dbh = new PDO('mysql:host=localhost;dbname=test', $user, $pass, array(
    PDO::ATTR_PERSISTENT => true
));
?>

@Shkarbatov
Copy link

http://www.php.net/pdo.connections

This is persistant connect, but it will not rescue when you will have connection timed out.

@samdark
Copy link
Member

samdark commented Sep 29, 2016

You should ping anyway in order to keep connection open even it's opened as persistent one.

@SilverFire
Copy link
Member

I don't think that there is any good and universal solution, because it depends on application specifics and developer is responsible for approach choosing.

I'm closing the issue, thanks to all for participating in this discussion.

@SilverFire SilverFire removed this from the 2.0.11 milestone Sep 29, 2016
@samdark
Copy link
Member

samdark commented Sep 29, 2016

So overall issue is the same when using both regular and persistent connection. You attempting making a query and DB server replies that it "has gone away". Sending additional query doesn't help the situation in any way. try-catch seems to be better solution but re-querying automatically could be dangerous if you use transactions so it seems overall it's very app-specific and could not be introduced at the framework level.

@Mirocow
Copy link
Author

Mirocow commented Sep 29, 2016

thx

@Shkarbatov
Copy link

Yes. In this case there should be additional try-catch with a failure handling that's application-specific. Right?

Yes, but this is crunch if we will cover each request.

So, the next way is extend Command with my example
#12634 (comment)

@samdark
Copy link
Member

samdark commented Sep 29, 2016

Indeed but your example could be dangerous if transactions are used. May lead to inconsistent DB state.

@Shkarbatov
Copy link

Why?

@samdark
Copy link
Member

samdark commented Sep 29, 2016

  1. Open transaction.
  2. Write post title change.
  3. Write comment 1.
  4. Lost connection. Transaction rolled back.
  5. Reopen connection.
  6. Write comment 2.

It was expected that post title change, comment 1 and comment 2 are written but only comment 2 was.

@Shkarbatov
Copy link

You can exclude this for transaction :)

For other ways it will help

@samdark
Copy link
Member

samdark commented Sep 29, 2016

You still need to reconnect for transactions but you need to repeat the whole transaction instead of part of it.

@SilverFire
Copy link
Member

I don't want to give a way to shoot yourself in the foot silently.

@samdark
Copy link
Member

samdark commented Sep 29, 2016

Yeah. That's tricky to solve in a good way for all cases.

@Shkarbatov
Copy link

Shkarbatov commented Sep 29, 2016

I don't want to give a way to shoot yourself in the foot silently.

Absolutely agry with you!

This is not issue for Yii2, this is crunch for current project ;)

@rob006
Copy link
Contributor

rob006 commented Sep 29, 2016

For Yii 1.1 I solve this in this way:

class ImportDbConnection extends CDbConnection {

    private $stamp;

    /**
     * {@inheritdoc}
     */
    public function createCommand($query = null) {
        $this->setActive(true);
        try {
            // send ping on every 10 seconds
            if ($this->stamp < time()) {
                $this->stamp = time() + 10;
                $ping = new CDbCommand($this, 'SELECT 1');
                $ping->execute();
            }
        } catch (CDbException $e) {
            // if ping fail, reconnect
            $this->setActive(false);
            $this->setActive(true);
        }
        return parent::createCommand($query);
    }

}

10 seconds could be parametrized and set by default to null, so reconnect will not work by default.

@samdark
Copy link
Member

samdark commented Sep 29, 2016

It won't send ping if there aren't queries for a minute, right?

@rob006
Copy link
Contributor

rob006 commented Sep 29, 2016

@samdark No, it will not prevent disconnecting, only try to reconnect if connection is lost. I use this for read only database for some long-running import scripts.

@samdark
Copy link
Member

samdark commented Sep 29, 2016

Yes. For read-only it's safe. I wonder why doing a special query instead of try-catch-ing the real one?

@rob006
Copy link
Contributor

rob006 commented Sep 29, 2016

CDbException can be thrown not only on disconnection - this is simplest way to ensure connection and avoid exception swallowing.

@samdark
Copy link
Member

samdark commented Sep 29, 2016

You can grep for "gone away", right?

@rob006
Copy link
Contributor

rob006 commented Sep 29, 2016

You can grep for "gone away", right?

This is dirty hack - what if I will have 'gone away' string in my query that failed and it will be part of error message? Or for some reason (localization?) in one of the servers message will be different? In my case the performance was not so crucial to waste time on the nuances.

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

No branches or pull requests

10 participants