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

Spring-Session does not allow the use of read replicas #3026

Open
osvso opened this issue Jun 11, 2024 · 4 comments
Open

Spring-Session does not allow the use of read replicas #3026

osvso opened this issue Jun 11, 2024 · 4 comments
Assignees
Labels
in: redis type: enhancement A general enhancement

Comments

@osvso
Copy link

osvso commented Jun 11, 2024

Describe the bug
At some point around the release of Spring Boot 3, we tracked changes in spring data in the hope that the base implementation would be redesigned, but unfortunately this did not happen. The existing implementation does not allow deterministic use of read replicas.
The problem is caused by the fact that when the session is first created, so a request is sent without a session identifier attribute (in any supported form), the implementation will attempt to save the session object twice.

	@Override
	public void save(RedisSession session) {
		if (!session.isNew) {
			String key = getSessionKey(session.hasChangedSessionId() ? session.originalSessionId : session.getId());
			Boolean sessionExists = this.sessionRedisOperations.hasKey(key);
			if (sessionExists == null || !sessionExists) {
				throw new IllegalStateException("Session was invalidated");
			}
		}
		session.save();
	}

On the first attempt, the session still has the isNew flag set to true, which causes it to be saved immediately.
The second save is triggered by the `SessionRepositoryFilter' as soon as the response is committed.

As a result, in the context of a single HTTP request, the service will first store the session in Redis (executed against the master node), then check if a session with a given id exists (executed against the read replica), and if it does, it will store it again. Otherwise it throws an IllegalStateException("Session was invalidated").

I've seen this issue was reported earlier by other developers, for more context please check the following items:

To Reproduce
Setup a simple app that uses Spring-Session with backed by Redis and read replicas.
I'm using AWS Elasticache, when read-replicas are enabled around 2% for requests fail due to replication not being fast enough.

Expected behavior
Spring-Session works efficiently and does not attempt to store the session data and almost immediately read right after.

@osvso osvso added status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Jun 11, 2024
@marcusdacoregio
Copy link
Contributor

Hi, @osvso. Thanks for the report.

As you already mentioned, the two saves are needed and well explained in the related issues and here.

However, I'm pretty sure that we could do better in such scenarios where replication is needed. Maybe one alternative would be to somehow track if the session has been created in the very same request and do not throw the IllegalStateException. Of course this would have to be an opt-in feature since we don't want to affect the current behavior.

Let me know what are your thoughts.

@marcusdacoregio marcusdacoregio self-assigned this Jun 25, 2024
@marcusdacoregio marcusdacoregio added type: enhancement A general enhancement in: redis and removed type: bug A general bug status: waiting-for-triage An issue we've not yet triaged labels Jun 25, 2024
@osvso
Copy link
Author

osvso commented Jul 12, 2024

Hi @marcusdacoregio!
Thank for your reply and pointing at the other issue. I believe it illustrates the use-case behind two saves neatly 👍

From replication stand-point, two saves is not a problem, but the intermediate lookup is. I'm not sure up to what degree it is a realistic to expect another request being executed referring the newly created session while the original request is still processing and making actual changes to the session itself. I believe it complicates a lot of things as we cannot guarantee the second request will get latest session view.

If we could use the session reference in the scenario when it is newly created through out the request processing time it should aid the replication issue. We can keep the saving mechanism intact.

@osvso
Copy link
Author

osvso commented Jul 19, 2024

Hello again @marcusdacoregio!

I find this problem as a bug and not really an enhancement as in the current form the solution cannot be used with replication enabled.
Also, we have just tested the ElastiCache Serverless integration from AWS and are experiencing issues from the get-go due to the internal replication - in this setup, we don't even need to configure the reader endpoint.

@marcusdacoregio
Copy link
Contributor

Hi @osvso, thanks for your reply, and sorry for the delay in getting back to you.

We have a couple of other enhancements prioritized for the next milestone release, but I'm planning to revisit this as soon as possible. However, if you are interested you can also start digging into the implementation to see how we can improve that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: redis type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

2 participants