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

Specify connection when creating Allocation/Admin session for Glacier2 #2966

Merged
merged 6 commits into from
Oct 30, 2024

Conversation

bernardnormier
Copy link
Member

@bernardnormier bernardnormier commented Oct 23, 2024

This PR updates the IceGrid code to specify the connection when creating "plain" sessions and Admin sessions via the Glacier2 session managers hosted by IceGrid.

It's not immediately clear why we didn't do it before. I believe the connections represent the correct connections. Maybe an oversight?

See also #2756 (loosely related).

See also #2967.

@pepone
Copy link
Member

pepone commented Oct 24, 2024

It's not immediately clear why we didn't do it before. I believe the connections represent the correct connections. Maybe an oversight?

In the original design, when a client sends a heartbeat to Glacier2, Glacier2 would send a keep-alive request to the target session. The lifetime of the session was tied to the Client-Glacier2 connection.

In Ice 3.7, when registering a session with a connection, the reap thread would add a HeartbeatCallbackI, which relied on ACM heartbeats to keep the session alive. However, when a session was created through Glacier2, we didn’t register it with a connection because the Glacier2-IceGrid connection isn’t tied to a single session.

The issue with the new setup is that, as long as a client keeps its session alive, the Glacier2-IceGrid connection remains open. This prevents the reaper from cleaning up any sessions created through Glacier2, as the connection doesn’t close when individual client sessions terminate.

@bernardnormier
Copy link
Member Author

The issue with the new setup is that, as long as a client keeps its session alive, the Glacier2-IceGrid connection remains open. This prevents the reaper from cleaning up any sessions created through Glacier2, as the connection doesn’t close when individual client sessions terminate.

In my view, that's not an issue.

When you create sessions using a Glacier2 router, it's the responsibility of the Glacier2 router to destroy these sessions, except in the case the Glacier2 router--IceGrid registry connection drops (in which case IceGrid cleans up sessions bound to the dropped connection).

Say we have:
client -> Glacier2 router -> IceGrid registry session manager

and the client crash. Then it's up to the Glacier2 router to "destroy" the session object hosted by IceGrid - or at least attempt to destroy it. And that's what Glacier2 router does:

connection->setCloseCallback(

router->destroy([self = shared_from_this()](exception_ptr e) { self->sessionDestroyException(e); });

if (_context.size() > 0)

cpp/src/IceGrid/ReapThread.cpp Outdated Show resolved Hide resolved
Copy link
Member

@bentoi bentoi left a comment

Choose a reason for hiding this comment

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

The connection close callback was added when ACM heartbeats were added (in 3.6). Prior to the addition of the connection close callback, sessions were only closed based on a keep alive Slice operation being called at regular intervals.

At this time, I believe we didn't want to break compatibility and still allow 3.5 clients to work with Glacier2 3.6 or vice-versa, ditto with different IceGrid/Glacier2 versions...

That's most likely the reason why we still have code that doesn't rely on the connection close callback for destroying sessions. And it's likely that we didn't consider removing this support with 3.7.

@bernardnormier bernardnormier merged commit 5407a60 into zeroc-ice:main Oct 30, 2024
17 checks passed
@bernardnormier bernardnormier deleted the grid-con branch December 9, 2024 17:10
InsertCreativityHere pushed a commit to InsertCreativityHere/compiler-comparison that referenced this pull request Jan 1, 2025
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.

4 participants