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

MYFACES-4660 4.1 view destroying fix + keeping track of user connection in multiple tabs #705

Open
wants to merge 2 commits into
base: 4.1.x
Choose a base branch
from

Conversation

milansie
Copy link
Contributor

1/ when the view is destroyed (i.e. the user reaches the NUMBER_OF_VIEWS_IN_SESSION limit), sessionScope channels are not destroyed. Channel could still be active for other views or newly created ones. In other words, session-scoped channels only get destroyed when the entire session is destroyed.

2/ we have to track number of user channelToken usage connections (when the user is using same channelToken in multiple browser windows / tabs with same http session, the channelToken remains same), and we can't close this channel until all user connections to this channel are closed (all tabs/windows).

When was the view scope destroyed, session scope channel tokens was also destroyed (although session was still alive). Destroying of the session scope channelToken is left to the automatic destroying of the SessionScope.

(cherry picked from commit 2e14d4c)
The number of channelToken used by user is tracked (increases when new connection is opened (in the same http session, ie new browser tab), decreased when the connection is closed). The channelToken is kept tracked until all connections are closed.

(cherry picked from commit b2cfa19)
@milansie milansie changed the title MYFACES-4660 view destroying fix + keeping track of user connection in multiple tabs MYFACES-4660 4.1 view destroying fix + keeping track of user connection in multiple tabs Apr 21, 2024
channelTokens.clear();
tokens.clear();
}
}
Copy link
Member

@tandraschko tandraschko Apr 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we still need to talk about this because of this comment:

The reason to do this is the connections must follow the same rules the view has, so if a view is disposed, all related websocket sessions must be disposed too on the server, and in that way we can avoid memory leaks.

i think we have to do some other mechanism here as basically its correct

you said:

any channelToken from the parent sessionScope is destroyed, even if it is still in use

shouldnt it only destroy it for the current view, which is now expired?
tokens.keySet() are the tokens from the current view, not just all in the session

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand it, channelToken (simple UUID String) is registered (put) in tokens Map in both, SessionScope and ViewScope bean (org.apache.myfaces.push.cdi.WebsocketScopeManager.AbstractUserScope#registerToken).

If you create new view (new ViewScope bean), under the same session (same SessionScope bean), same token is used.
When the ViewScope bean is destroyed, you want to clear tokens for this bean - that's ok. But you don't want to clear tokens from other View beans, do you?

What happened when you create new view no NUMBER_OF_VIEWS_IN_SESSION+1. You register channelToken (already existing token) for this ViewScope and SessionScope (WebsocketComponentRenderer is responsible to that).
Then some mechanism decides, that very first View should be destroyed (View no1). Method onDestroy is called on ViewScope bean, which clears tokens from ViewScope (ok), but (in previous implementation) also from SessionScope bean.
And finally, client browser tried to open new WebSocket session (component is rendered on client side, websockect connection is created from the client side). But this fails, while it can find sessionScope channelToken, which was removed in previous step (now I can't remember exactly where if failed, we faced that issue few weeks ago / but I think it could be easily simulated).

And I think, that our mechanism has nothing to do with real websocket connection. These are managed by application container (tomcat, ...), and are creating/destroying automatically (we are just rendering the component, and reacting to onOpen/onClose events, and of course, we are sending messages).
So, what we do have here is just to handle channel names and channel tokens linked to websockets connections. When application container is working correctly, the worst what could happen (IMHO) is having channelTokens reference to non-existing websocket session until the SessionScope is destroyed.

Otherwise, we have to think about destroying the sessionScope channelTokens only when we are 100% sure, that no other ViewScope is using that.

I'm ready for discussion :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am able to simulate this behavior, destroying viewScope comes from SerializedViewCollection, where at the end of method put is something like destroyCallback.accept(oldViewScopeId);, where the viewScope is destroyed (and also sessionScope channels).
Since then we are not able to register new websocket connection org.apache.myfaces.push.EndpointImpl#onOpen, sessionManager.addOrUpdateSession(channelToken, session) fails and websocket connection is closed with CloseCodes.UNEXPECTED_CONDITION

@tandraschko
Copy link
Member

tandraschko commented Apr 24, 2024

how are the channelTokens generated? i wonder that if you open index.xhtml in two different tabs, does it use the same channelToken?
this creates 2 slots in the "NUMBER_OF_VIEWS_IN_SESSION". If one is closed, only the viewscope bean of this one should and the channel of this should be closed. Not both.

@milansie
Copy link
Contributor Author

Yup, it seems so. If the channel is applicationScope, only one channelToken exists for the application.
If the channel scope is session (and user is specified), there is same channelToken with two websocket sessions connected.

you are right, only one ViewScope bean is closed. But session channelToken was also removed (not destroying the session, only removing the channelToken from sessionScope bean), and therefore registering new websocket session for this user session fails by websocket eror code 1011

how are the channelTokens generated? i wonder that if you open index.xhtml in two different tabs, does it use the same channelToken? this creates 2 slots in the "NUMBER_OF_VIEWS_IN_SESSION". If one is closed, only the viewscope bean of this one should and the channel of this should be closed. Not both.

@tandraschko
Copy link
Member

TBH i would some time for debugging and i dont have this

maybe just the channelToken generation is wrong, dont know currently, sorry

@tandraschko
Copy link
Member

@milansie do you have discord? we could chat about this in the PF discord
i would get rid of open PRs ;D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants