Skip to content

🐛 Fixes possible issue with GC closing projects #6492

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.

Sign up for GitHub

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

Closed
wants to merge 5 commits into from
Closed

🐛 Fixes possible issue with GC closing projects #6492

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b70a5cc
fix alive key left behind after socket disconnection
Oct 5, 2024
5fce9e7
remove comment
Oct 7, 2024
437b95b
refactor
Oct 7, 2024
983b8a5
revert
Oct 7, 2024
bf4f248
revert
Oct 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions services/web/server/src/simcore_service_webserver/resource_manager/user_sessions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ async def user_pressed_disconnect(self) -> None:

await self._registry.set_key_alive(self._resource_key(), 1)

async def remove_socket_id(self) -> None:
async def remove_socket_id_after_disconnection(self) -> None:
Copy link
Member

@sanderegg sanderegg Oct 14, 2024

Choose a reason for hiding this comment

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

I don't why you change the naming of that function.
In effect you are moving logic naming into a deeper location.

_logger.debug(
"user %s/tab %s removing socket from registry...",
self.user_id,
Expand All @@ -117,9 +117,9 @@ async def remove_socket_id(self) -> None:
)

await self._registry.remove_resource(self._resource_key(), _SOCKET_ID_FIELDNAME)
await self._registry.set_key_alive(
self._resource_key(), _get_service_deletion_timeout(self.app)
)
# when the tab is closed the alive key is also removed immediately,
Copy link
Collaborator

@matusdrobuliak66 matusdrobuliak66 Oct 7, 2024

Choose a reason for hiding this comment

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

So from my understanding, this issue is somehow connected to session IDs (where there is probably some issue how we work with them cross product/cross tabs) did you test it and are you sure this removal will not cause other side-effect issue? Are you able to reproduce this issue? (its happening so often that I guess it should be reproducable?)

Copy link
Contributor Author

@GitHK GitHK Oct 7, 2024

Choose a reason for hiding this comment

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

You can reproduce this everywhere. Open a project and cose the tab its alive key will still be present in Redis. This makes no sense to me. But I might be wrong.

Copy link
Member

@pcrespov pcrespov Oct 7, 2024

Choose a reason for hiding this comment

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

If you are not sure, why are you changing it without verifying? What if this change makes the GC more unstable?

# there is no reason to keep it active
await self._registry.set_key_alive(self._resource_key(), 1)
Copy link
Member

@sanderegg sanderegg Oct 14, 2024

Choose a reason for hiding this comment

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

the key alive has a TTL.
I checked that this function _get_service_deletion_timeout (which is badly named) returns the default TTL.
Did you actually check what happens when you close the tab now? it will instantly remove the service when the GC runs.

This kind of disconnection happen a lot in a train for example... so I think here you might be messing around with it. Did you test that use-case?


async def set_heartbeat(self) -> None:
"""Extends TTL to avoid expiration of all resources under this session"""
Expand Down
4 changes: 2 additions & 2 deletions services/web/server/src/simcore_service_webserver/socketio/_handlers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ async def disconnect(socket_id: SocketID, app: web.Application) -> None:
f"{client_session_id=}",
):
with managed_resource(user_id, client_session_id, app) as user_session:
await user_session.remove_socket_id()
await user_session.remove_socket_id_after_disconnection()
# signal same user other clients if available
await emit(
app,
Expand All @@ -192,7 +192,7 @@ async def disconnect(socket_id: SocketID, app: web.Application) -> None:
else:
# this should not happen!!
_logger.error(
"Unknown client diconnected sid: %s, session %s",
"Unknown client disconnected sid: %s, session %s",
socket_id,
f"{socketio_session}",
)
Expand Down
2 changes: 1 addition & 1 deletion services/web/server/tests/unit/with_dbs/01/test_resource_manager_user_sessions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,7 @@ async def test_users_sessions_resources_registry(
client_session_id = tabs[socket_id]

with managed_resource(user_id, client_session_id, redis_enabled_app) as rt:
await rt.remove_socket_id()
await rt.remove_socket_id_after_disconnection()

num_sockets_for_user = len(await rt.find_socket_ids())
assert num_sockets_for_user == (NUM_SOCKET_IDS - socket - 1)
Expand Down
Loading