googleapis / google-cloud-php

Google Cloud Client Library for PHP
https://cloud.google.com/php/docs/reference
Apache License 2.0
1.09k stars 436 forks source link

[Spanner] Re-create sessions that have been invalidated by the server #6284

Closed taka-oyama closed 1 year ago

taka-oyama commented 1 year ago

I was reading through the documentation on Sessions and noticed the following line.

Attempts to use a deleted session result in NOT_FOUND. If you encounter this error, create and use a new session, add the new session to the pool, and remove the deleted session from the pool.

Currently, this is not possible from userland because there is no public method available to delete the current session.

Would it be possible to handle this on the library side?

I found that this is handled by the library in google-cloud-java https://github.com/googleapis/google-cloud-java/pull/4734. Here is another example from google-cloud-go https://github.com/googleapis/google-cloud-go/issues/1527.

Creating a case where the server throws a "NOT FOUND" exception can be reproduced by following the steps on #5827.

vishwarajanand commented 1 year ago

Do you have scenarios when there are accumulated deleted sessions? I see it as a large effort to implement (potentially changes in a lot of places) and comparatively very low effort to mitigate at user end.

taka-oyama commented 1 year ago

Hi, thanks for looking into the issue.

Do you have scenarios when there are accumulated deleted sessions?

We have seen this error numerous times in production through out the years on queue workers. It's not very clear why this happens, so I will try go find out. This may take some time.

Btw, if you look at https://github.com/colopl/laravel-spanner/issues/37, https://github.com/googleapis/google-cloud-php/issues/1808, at least 4 other people have encountered the same error.

very low effort to mitigate at user end.

In order to fix this, we had to make many changes to our existing code.

It took way more effort than it should have because, we couldn't find a way to delete a single session from the session pool. The only way to resolve it was to delete the entire session pool which was less than ideal. We wasted many hours testing and confirming that clearing the entire cache wouldn't break anything.

Would it be possible to at least expose a method which will allow end user to delete the currently used session?

vishwarajanand commented 1 year ago

@taka-oyama I can reproduce NOT_FOUND errors easily by deleting the session before making the call. But I need to repro or showcase scenario where a deleted sessions are present in the CacheSessionPool even after calling maintain regularly. Currently, it seems to be happening only inside laravel.

taka-oyama commented 1 year ago

One generic case where NOT_FOUND is thrown even when maintain is called is the following case.

Below is the code I used to confirm this.

$sessionPool = new CacheSessionPool(
    new SysVCacheItemPool(),
    ['minSessions' => 1],
);

$database = (new SpannerClient())->connect(
    "my-instance",
    "my-database",
    ['sessionPool' => $sessionPool],
);

$database->execute('SELECT 1')->rows()->current();

$timeout = strtotime('+2 hours');

while($timeout > time()) {
    $sessionPool->maintain();
    sleep(1800); // 30 min
}

$database->execute('SELECT 1')->rows()->current();

This happens because the session is never returned to the session pool.

I understand that session should be returned by closing the connection when the connection is idle but I'm afraid most people don't realize it needs to be done.

vishwarajanand commented 1 year ago

Hi @taka-oyama, maintain renews the sessions that are about to go stale in next ~10 mins. So, if you call the above code with sleep(600), I think the failures won't happen.

Yet to test this though.

vishwarajanand commented 1 year ago

Re-tested this and it seems even with a reduced sleep time, maintain seems to miss refreshing the session.

vishwarajanand commented 1 year ago

I think I know what's happening here. maintain is responsible for refreshening up the session in queue but not inuse. I think this is by design, but it is a blocker for someone who's using session pool in a long running script with lots of sleep and infrequent DB operations.

Checking internally whether there's any particular reason to whether we should purge/refresh in-use sessions.

vishwarajanand commented 1 year ago

So, it seems maintain is not supposed to handle the inUse sessions and thats by design. A plausible workaround (agreed that its not the ideal or desired) is to re-craete the Database object when using in a thread which sleeps >1 hr.

Closing since no action.