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

[6.x] Prevent making actual pdo connections while reconnecting #30998

Merged
merged 2 commits into from Jan 2, 2020

Conversation

@themsaid
Copy link
Member

themsaid commented Jan 2, 2020

Calling $connection->getPdo() or $connection->getReadPdo() starts an actual connection with the database:

Problems appear when the connection we are using is a read-only or write-only connection, in that case we don't want to make the other PDO connection while it won't be used as seen in this issue(#30957).

I went for readRawPdo and readWritePdo for method names as we have the same in Query\Builder.php where we have a getter getBidnings and another method getRawBindings. That way we won't have to break the interface.

@bernardwiesner

This comment has been minimized.

Copy link

bernardwiesner commented Jan 2, 2020

Thanks for the PR.

What I wonder about this implementation though is that now DB::reconnect() is not really reconnecting to the DB but just preparing the connections and the reconnect only really happens when a select or insert is executed.

@themsaid

This comment has been minimized.

Copy link
Member Author

themsaid commented Jan 2, 2020

@bernardwiesner I think that's the intended way for it to work. Why make a connection before actually needing one?

@bernardwiesner

This comment has been minimized.

Copy link

bernardwiesner commented Jan 2, 2020

It just seems that the comments are rather misleading now, since disconnect() has:

     * Disconnect from the given database.

which actually does disconnect the connection and reconnect() has:

     * Reconnect to the given database.

which would not actually reconnect to the DB.

To me what makes more sense is to have an implementation where if a previous write connection existed and you call reconnect you only reconnect the write, if you only had a read, only the read is reconnected. If you had both, then both get reconnected.

@themsaid

This comment has been minimized.

Copy link
Member Author

themsaid commented Jan 2, 2020

@bernardwiesner, when you disconnect we don't store if the read/write connections are in place. That's why there's no way we know what this connection used to do. To me a fresh connection is safer and better approach.

@bernardwiesner

This comment has been minimized.

Copy link

bernardwiesner commented Jan 2, 2020

You think something like this would work? I am assuming that when the PDOs are set to null it means they were disconnected.

DatabaseManager.php

    protected function refreshPdoConnections($name)
    {
        $fresh = $this->makeConnection($name);        
        if($this->connections[$name]->wasWriteDisconnected()){
            $this->connections[$name]->setPdo($fresh->getPdo());
        }
        if($this->connections[$name]->wasReadDisconnected()){
            $this->connections[$name]->setReadPdo($fresh->getReadPdo());
        }
        return $this->connections[$name];
    }

Connection.php

    protected function reconnectIfMissingConnection()
    {
        if (is_null($this->pdo) || is_null($this->readPdo)) {
            $this->reconnect();
        }
    }
    
    public function disconnect()
    {
        if ($this->isWriteConnected()) {
            $this->setPdo(null);
        }
        if ($this->isReadConnected()) {
            $this->setReadPdo(null);
        }
    }

    public function wasWriteDisconnected(){
        return is_null($this->pdo);
    }

    public function wasReadDisconnected(){
        return is_null($this->readPdo);
    }

    public function isWriteConnected(){
        if ($this->pdo instanceof Closure) {
            return false;
        }
        return true;
    }

    public function isReadConnected(){
        if ($this->readPdo instanceof Closure) {
            return false;
        }
        return true;
    }
@themsaid

This comment has been minimized.

Copy link
Member Author

themsaid commented Jan 2, 2020

@bernardwiesner again, why make the connection before actually needing one? Your app could terminate without making any select/insert queries, why make a connection without using it?

Can you explain a use case?

@bernardwiesner

This comment has been minimized.

Copy link

bernardwiesner commented Jan 2, 2020

Yeah I also cant think of many use cases, I just thought the comments and namings were a little off now. The only use case I can think of is an asynchronous one, where you may want to save some time on the reconnect and call it while processing some other data.

@taylorotwell taylorotwell merged commit a47cd1b into laravel:6.x Jan 2, 2020
2 checks passed
2 checks passed
continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.