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

fix: Initialize lastConnectionCheck after first connection #50874

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

artonge
Copy link
Contributor

@artonge artonge commented Feb 18, 2025

Summary

We are checking whether the DB connection is alive once every 30 seconds. But when we are lacking the last check time, we are skipping the check and reconnect logic. This is causing the reconnect logic to never fire in those cases.

Investigation

It seems to me that "those cases", are actually always the case, as upon initialization, we are not using the proper connection name to store the time.

In the connect() logic, when $this->_conn is null, $this->getConnectionName() is returning replica, so $this->lastConnectionCheck will be equal to ['replica' => time()];

if ($this->_conn) {
$this->reconnectIfNeeded();
/** @psalm-suppress InternalMethod */
return parent::connect();
}
$this->lastConnectionCheck[$this->getConnectionName()] = time();

private function getConnectionName(): string {
return $this->isConnectedToPrimary() ? 'primary' : 'replica';
}

https://github.com/nextcloud/3rdparty/blob/2b6d7bf65ff242ea050e736925f752a38d8da220/doctrine/dbal/src/Connections/PrimaryReadReplicaConnection.php#L136-L139

Then, if the connection name ends up as being 'primary', the reconnect logic is skipped:

if (
!isset($this->lastConnectionCheck[$this->getConnectionName()]) ||
time() <= $this->lastConnectionCheck[$this->getConnectionName()] + 30 ||
$this->isTransactionActive()
) {
return;
}

Other solution

Find a more reliable way to get the connection name.

Follow-up of #41819

@artonge artonge force-pushed the artonge/fix/login_flow_v2_sessions branch 2 times, most recently from 8e5a9e6 to cd5cdfd Compare February 18, 2025 11:11
@artonge artonge self-assigned this Feb 18, 2025
@artonge artonge added bug 3. to review Waiting for reviews php Pull requests that update Php code feature: database Database related DB labels Feb 18, 2025
@artonge artonge added this to the Nextcloud 31 milestone Feb 18, 2025
@come-nc
Copy link
Contributor

come-nc commented Feb 18, 2025

  • Remove the $this->lastConnectionCheck initialization code as it is useless?

Maybe move it down after the call to parent::connect? There getConnectionName should return the correct value, no?

@artonge
Copy link
Contributor Author

artonge commented Feb 18, 2025

Maybe move it down after the call to parent::connect? There getConnectionName should return the correct value, no?

That makes sense indeed.

@artonge artonge force-pushed the artonge/fix/login_flow_v2_sessions branch from cd5cdfd to 10c0c78 Compare February 18, 2025 14:50
@artonge artonge changed the title fix: Do not skip reconnect if last check is unknown fix: Initialize lastConnectionCheck after first connection Feb 18, 2025
We are checking whether the DB connection is alive once every 30 seconds. But when we are lacking the last check time, we are skipping the check and reconnect logic. This is causing the reconnect logic to never fire in those cases.

It seems to me that "those cases", are actually always the case, as upon initialization, we are not using the proper connection name to store the time.

In the `connect()` logic, when `$this->_conn` is null, `$this->getConnectionName()` is returning `replica`, so `$this->lastConnectionCheck` will be equal to `['replica' => time()];`

https://github.com/nextcloud/server/blob/60711ea4cfde6f53d0b18bcd7e166a34a43056a5/lib/private/DB/Connection.php#L215-L221

https://github.com/nextcloud/server/blob/60711ea4cfde6f53d0b18bcd7e166a34a43056a5/lib/private/DB/Connection.php#L891-L893

https://github.com/nextcloud/3rdparty/blob/2b6d7bf65ff242ea050e736925f752a38d8da220/doctrine/dbal/src/Connections/PrimaryReadReplicaConnection.php#L136-L139

Then, if the connection name ends up as being 'primary', the reconnect logic is skipped:

https://github.com/nextcloud/server/blob/60711ea4cfde6f53d0b18bcd7e166a34a43056a5/lib/private/DB/Connection.php#L874-L880

Follow-up of #41819

Signed-off-by: Louis Chemineau <louis@chmn.me>
@artonge artonge force-pushed the artonge/fix/login_flow_v2_sessions branch from 10c0c78 to 066c92f Compare February 19, 2025 11:25
@artonge
Copy link
Contributor Author

artonge commented Feb 19, 2025

Confirmed to improve the situation in production.

@Altahrim Altahrim mentioned this pull request Feb 20, 2025
5 tasks
@artonge artonge modified the milestones: Nextcloud 31, Nextcloud 32 Feb 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug feature: database Database related DB php Pull requests that update Php code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants