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

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

Closed
taka-oyama opened this issue May 29, 2023 · 8 comments
Closed
Assignees
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@taka-oyama
Copy link
Contributor

taka-oyama commented May 29, 2023

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 googleapis/google-cloud-java#4734.
Here is another example from google-cloud-go googleapis/google-cloud-go#1527.

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

@vishwarajanand vishwarajanand added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. priority: p2 Moderately-important priority. Fix may not be included in next release. labels Jun 12, 2023
@vishwarajanand vishwarajanand self-assigned this Jun 12, 2023
@vishwarajanand
Copy link
Contributor

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
Copy link
Contributor Author

taka-oyama commented Jun 13, 2023

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 colopl/laravel-spanner#37, #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
Copy link
Contributor

@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
Copy link
Contributor Author

taka-oyama commented Jun 14, 2023

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

  • create a connection (SpannerClient::connect(...))
  • execute a query
  • keep the connection and stay idle for 1 hour+ (session closed by server)
  • execute a query (throws NotFoundException)

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
Copy link
Contributor

vishwarajanand commented Jun 27, 2023

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
Copy link
Contributor

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

@vishwarajanand vishwarajanand added the status: investigating The issue is under investigation, which is determined to be non-trivial. label Jul 5, 2023
@vishwarajanand
Copy link
Contributor

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

No branches or pull requests

2 participants