ReactiveRedisOperationsSessionRepository logout race condition with NullPointerException in findById

[haizz]

Using Spring Boot 2.0.3 with reactive WebFlux and spring-session-data-redis 2.0.4.

1. Suppose we have a long-running web request (for example, WebSocket).
2. Execute logout operation while long-running request 1 is still running.
3. Session changes its key in Redis.
4. When request 1 finishes an operation to save session delta will be executed.
5. ReactiveRedisOperationsSessionRepository saves changed fields (for example, lastAccessedTime) USING OLD SESSION KEY.
6. So now we have OLD SESSION KEY back in Redis having only one field: lastAccessedTime.
7. Any new request which uses OLD SESSION KEY fails in ReactiveRedisOperationsSessionRepository.findById with NullPointerException in SessionMapper because all other fields like creationTime are missing.
8. Panic ensues.

EDIT: Created related Spring Framework issue: https://jira.spring.io/browse/SPR-17051

[vpavic]

Thanks for the report @haizz.

I see your point, and that this can be a problematic scenario. We have a similar report with HttpSession and JDBC in #1031.

[vpavic]

Also note that there's a similar problem (though one that does not involve race condition) with the same side effect reported in #1114.

[haizz]

@vpavic is there some kind of a workaround until this thing is fixed?

[haizz]

@vpavic Will it be fixed in 2.0.5? My app is actively using WebSockets, so production usage of reactive Redis sessions is impossible for me.

[vpavic]

@haizz I've resolved #1114 today, which is related to your issue. With that fix in place, I haven't been able to reproduce your problem. Could you please give 2.0.5.BUILD-SNAPSHOT a try and let me know if you still experience the issue described in your original comment? Thanks.

[haizz]

@vpavic thanks! Unfortunately, nothing changed for me. Probably because Spring Security WebSessionServerSecurityContextRepository calls changeSessionId() on logout rather than invalidate()

[vpavic]

Thanks for following up @haizz. I've been using Spring Security's standard logout mechanism as well in my tests.

Are you sure you tried with 2.0.5.BUILD-SNAPSHOT for the spring-session-core module (since that's what's affected by changes from #1114) and not only spring-session-data-redis?

[haizz]

@vpavic Yes, I even placed some breakpoints before invoking logout request:

[haizz]

Actually, what I have now:

1. Two WebSocket tabs in the browser.
2. Each WebSocketHandler calls ReactiveRedisOperationsSessionResitory.findById periodically to check if the session is still alive to shutdown the websocket if it's not.
3. Logout in one tab.
4. Next ReactiveRedisOperationsSessionResitory.findById call fails with NPE in the second WebSocketHandler.

[haizz]

3.5. One WebSocket shuts down because the session doesn't exist anymore with old key.
3.6. lastAccessTime is saved to Redis with old key.

[vpavic]

Thanks for further info @haizz. Any chance you could put this into a sample app that we could use to reproduce the problem?

[haizz]

@vpavic https://github.com/haizz/sessionbugdemo

[vpavic]

Thanks for the sample app @haizz, I think I now have an idea what's going on.

The problem is that the WebSocket request is a long running one, and as such DefaultWebSessionManager#getSession retrieves the session that was valid at the start of the request, however when request is being committed DefaultWebSessionManager#save gets invoked with a session that doesn't exist anymore, or to be more precise, the one that had it's session id changed in the logout event that has happened in the meantime.

There are actually two problematic places here:

• DefaultWebSessionManager#save checks whether previously retrieved session is active (started) and not expired, but doesn't check if it actually exists in the session store
• ReactiveRedisOperationsSessionRepository#save doesn't validate if the session passed in exists in the store, and just applies the delta (which in this case is only lastAccessedTime), which results in a session entry with only lastAccessedTime attribute

For the first part, i.e. DefaultWebSessionManager, please raise an issue with Spring Framework describing this scenario. Feel free update this issue with a link to that one.

The second part of the story (ReactiveRedisOperationsSessionRepository) is on our side.

[haizz]

Thanks @vpavic! Will the second part be fixed in 2.0.5?

[haizz]

Created issue with Spring Framework: https://jira.spring.io/browse/SPR-17051

[vpavic]

@haizz This is now resolved and will be available in 2.1.0.M3 and 2.0.6.RELEASE via #1185.