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

SockJS Event Bus Bridge does not keep web sessions alive for clustered session managers #2600

Open
cgm-aw opened this issue Apr 25, 2024 · 2 comments
Assignees
Labels
Milestone

Comments

@cgm-aw
Copy link

cgm-aw commented Apr 25, 2024

Version

>= 4.5.0 (older are probably also affected)

Context

When using the SockJS event bus bridge, EventBusBridgeImpl#internalHandlePing keeps the web session alive by calling Session#setAccessed
However this only works for a local session storage. ClusteredSessionStoreImpl does not honor Session#lastAccessed but rather uses the TTL feature of AsyncMap.

Do you have a reproducer?

Yes please check out https://github.com/cgm-aw/vertx-sessiontimeout-reproducer

Steps to reproduce

  1. Deploy a verticle with a clustered session manager
  2. Perform a http call, you will get a web session
  3. Open a SockJS connection and utilize the event bus bridge so the periodic ping happens
  4. Wait until the configured session timeout is over
  5. Perform another http call, you will get a new web session <- the ping should have kept the original session alive

Please check out my reproducer, you can reproduce the issue and also see how the mechanism works fine with the local session storage.

Best regards

@cgm-aw cgm-aw added the bug label Apr 25, 2024
@tsegismont tsegismont self-assigned this May 24, 2024
@tsegismont tsegismont added this to the 4.5.9 milestone May 24, 2024
@vietj vietj modified the milestones: 4.5.9, 4.5.10 Jul 17, 2024
@tsegismont
Copy link
Contributor

I spent some time looking into this and here are my findings.

Calling Session.setAccessed in EventBusBrideImpl was added in #382 (Vert.x 3.3). It hasn't been documented nor tested. It leverages an implementation detail of the LocalSessionStore to extend the lifetime of a web Session: this store uses the last accessed time to determine if session data should be removed from memory.

Other session stores (clustered session store, Redis, Infinispan) use TTL attributes of the underlying storage to expire a session. The lifetime is only extended when a session is flushed.

A session is flushed after the HTTP response headers have been sent if:

  • the session has been accessed using RoutingContext#session, or
  • if the sessionHandler is not configured for lazy session creation.

So, this behavior can be seen not only with the websocket transport but also with other transports, provided the session handler is lazy and the session is not accessed.

Note that raw WebSocket message exchanges (without SockJS) don't extend the lifetime of a web Session either.

I err on the side of closing this as won't fix. I would advise users who want to extend the lifetime of a session to make the browser send a request to the backend.

Thoughts @vietj @pmlopes ?

@cgm-aw
Copy link
Author

cgm-aw commented Sep 3, 2024

Thank you for our analysis, @tsegismont!
I would be a bit sad if you closed this with "won't fix" - as long as the web socket connection is open, it clearly means that the client is still alive. In my mind, the client would have to actively close the web socket connection (and therefore the backend must destroy the session), if the user logged out.
Introducing a second keep-alive via http in parallel to the web socket keep-alive would be annoying for the client.

Best regards

@vietj vietj modified the milestones: 4.5.10, 4.5.11 Sep 4, 2024
@vietj vietj modified the milestones: 4.5.11, 4.5.12 Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

3 participants