Skip to content

Conversation

@pepone
Copy link
Member

@pepone pepone commented Oct 29, 2025

No description provided.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a new Glacier2 Session demo to the C++ demos repository. The demo illustrates how to implement and configure a Session Manager with Glacier2, demonstrating session-based access control and per-user state management using a Pokémon-themed application.

  • Implements a complete session management system with Glacier2 router integration
  • Demonstrates the use of a shared servant pattern for implementing multiple objects
  • Includes client-server communication through a Glacier2 router with session-based authentication

Reviewed Changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
cpp/Glacier2/session/config.glacier2 Glacier2 router configuration file with client/server endpoints and session manager settings
cpp/Glacier2/session/SharedPokeBox.h Header for shared servant implementing the PokeBox interface
cpp/Glacier2/session/SharedPokeBox.cpp Implementation of shared PokeBox servant with session-based user ID resolution
cpp/Glacier2/session/SessionManager.h Header for SessionManager servant that creates sessions and resolves user IDs
cpp/Glacier2/session/SessionManager.cpp Implementation of SessionManager with session token management
cpp/Glacier2/session/Server.cpp Server application that hosts the SessionManager and SharedPokeBox
cpp/Glacier2/session/README.md Documentation for building and running the demo
cpp/Glacier2/session/PokeBox.ice Slice definitions for PokeBox and PokeSession interfaces
cpp/Glacier2/session/InMemoryPokeStore.h Header for in-memory Pokémon storage implementation
cpp/Glacier2/session/InMemoryPokeStore.cpp Implementation of in-memory Pokémon storage
cpp/Glacier2/session/IUserIdResolver.h Interface for resolving user IDs from session tokens
cpp/Glacier2/session/IPokeStore.h Interface for Pokémon storage operations
cpp/Glacier2/session/DefaultPokeSession.h Header for PokeSession servant implementation
cpp/Glacier2/session/DefaultPokeSession.cpp Implementation of PokeSession with session lifecycle management
cpp/Glacier2/session/Client.cpp Client application demonstrating session creation and usage
cpp/Glacier2/session/CMakeLists.txt CMake build configuration for the demo
cpp/Glacier2/session/.gitignore Git ignore file for build artifacts

Copy link
Member

@externl externl left a comment

Choose a reason for hiding this comment

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

Looks good!

namespace Server
{
/// Represents a Pokémon storage system.
class IPokeStore

Choose a reason for hiding this comment

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

I don't think we use the I prefix for interfaces convention in C++, do we?
This should probably just be PokeStore like we do in the non-C# languages.

#define IPOKE_STORE_H

#include "PokeBox.h"
#include <list>

Choose a reason for hiding this comment

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

Do we need to #include list here? We never use list, just PokemonList from Pokebox.h.
Which I think gets mapped to an std::vector anyways by default.

Suggested change
#include <list>

namespace Server
{
/// Resolves a user ID from a session token.
class IUserIdResolver

Choose a reason for hiding this comment

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

Same, I feel like in C++ we'd just use UserIdResolver.

public:
/// Gets the user ID associated with the specified session token.
/// @param token The session token.
/// @returns The user ID associated with the specified session token, or null if not found.

Choose a reason for hiding this comment

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

Since this returns an optional and not a pointer.

Suggested change
/// @returns The user ID associated with the specified session token, or null if not found.
/// @returns The user ID associated with the specified session token, or nullopt if not found.


using IPokeStorePtr = std::shared_ptr<IPokeStore>;
}
#endif

Choose a reason for hiding this comment

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

Usually an extra line separating the endif from the actual code.

Suggested change
#endif
#endif

Comment on lines +33 to +34
/// @param current Information about the incoming request being dispatched.
std::string getUserId(const Ice::Current& current) const;

Choose a reason for hiding this comment

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

Doesn't have an @returns. To be fair, this looks like it was also forgotten in the C# demo too.

Suggested change
/// @param current Information about the incoming request being dispatched.
std::string getUserId(const Ice::Current& current) const;
/// @param current Information about the incoming request being dispatched.
/// @returns The user ID associated with the current session.
std::string getUserId(const Ice::Current& current) const;

std::optional<Glacier2::SessionControlPrx> sessionControl,
const Ice::Current& current) final;

std::optional<std::string> getUserId(const std::string& token) final;

Choose a reason for hiding this comment

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

Can this be const like the other getters?

Suggested change
std::optional<std::string> getUserId(const std::string& token) final;
std::optional<std::string> getUserId(const std::string& token) const final;

void
DefaultPokeSession::destroy(const Ice::Current& current)
{
std::cout << "Destroying session #" << current.id.name << std::endl;

Choose a reason for hiding this comment

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

We're using std in this file. Also we didn't qualify cout or endl in SharedPokebox.cpp either.
So for consistency:

Suggested change
std::cout << "Destroying session #" << current.id.name << std::endl;
cout << "Destroying session #" << current.id.name << endl;

Comment on lines +57 to +58
> You can also start the Glacier2 router before the server. The order does not matter: the server is identical to the
> server provided in the [Ice Greeter][1] demo and does not depend on Glacier2.

Choose a reason for hiding this comment

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

This is the comment we use for the other Glacier2 session demos:

Suggested change
> You can also start the Glacier2 router before the server. The order does not matter: the server is identical to the
> server provided in the [Ice Greeter][1] demo and does not depend on Glacier2.
> You can also start the Glacier2 router before the server. The order does not matter.

The rest is leftover from the Glacier2 greeter demo.
I did the same thing when writing the Java session demo : vP


If you don't specify a name, the client uses the current username.
[1]: ../../Ice/Greeter

Choose a reason for hiding this comment

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

And then this isn't necessary anymore, since we don't reference the Greeter demo.

Suggested change
[1]: ../../Ice/Greeter

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.

3 participants