Fix proxy usage and creation in IceGrid implementation #1933

pepone · 2024-03-14T12:33:57Z

No description provided.

externl

Looks good to me!

externl · 2024-03-14T17:23:20Z

cpp/src/IceBox/ServiceManagerI.cpp

+    // Null observers are ignored
+    if (observer)
+    {
+        addObserver(*observer);


Should we move it out of the optional?

Yes we should.

externl · 2024-03-14T17:34:34Z

cpp/src/IceGrid/AdminSessionI.cpp

+        name == _replicaName ? _database->getReplicaCache().getInternalRegistry()
+                             : _database->getReplicaCache().getInternalRegistry(),


Shouldn't the false case be _database->getReplica(std::move(name))->getProxy();

externl · 2024-03-14T17:49:12Z

cpp/src/IceGrid/LocatorRegistryI.cpp

-        // the server is released during the server startup.
-        //
-        ServerPrxPtr server;
+        // NOTE: We pass false to the getServer call to indicate that we don't necessarily want an up-to-date adapter


Not specifically related to your PR, but where are we passing false to getServer?

I think the comment refers to the the getProxy call in getServer(id)->getProxy(false);

externl · 2024-03-14T17:59:38Z

cpp/src/IceGrid/RegistryI.cpp


-        ObjectPrxPtr proxy;
+        optional<ObjectPrx> proxy;
        try
        {
            proxy = _database->getObjectProxy(id);


Should getObjectProxy be updated to not return an optional?

I don't think so. This way we know that we are getting non null proxies from the database.

externl · 2024-03-14T18:06:46Z

cpp/src/IceGrid/ServerAdapterI.h

            const std::string&,
            bool);
        ~ServerAdapterI() override;

        void activateAsync(
-            std::function<void(const Ice::ObjectPrxPtr&)>, // TODO: pass by value!
+            std::function<void(const std::optional<Ice::ObjectPrx>&)>, // TODO: pass by value!


Is this TODO still relevant?

Yes, the callback is still taking a reference as before.

externl · 2024-03-14T18:09:27Z

cpp/src/IceGrid/SessionManager.h

                            }
                            break;
                        case Disconnect:
                            assert(session);
-                            destroySession(session);
+                            destroySession(*session);


How do we now session is not null here (and below)?

The previous code already has an assert for this.

externl · 2024-03-14T18:12:36Z

cpp/src/IceGrid/WellKnownObjectsManager.cpp

-    Ice::Identity id = {"Locator", _database->getInstanceName()};
-    return Ice::uncheckedCast<LocatorPrx>(getWellKnownObjectReplicatedProxy(std::move(id), "Client"));
+    return LocatorPrx{
+        getWellKnownObjectReplicatedProxy(Ice::Identity{"Locator", _database->getInstanceName()}, "Client")};


Is Ice::Identity required here?

Not sure, but IMO it is more readable.

bernardnormier

Looks good, but the Slice file doc-comments need some fixes.

bernardnormier · 2024-03-14T21:06:32Z

cpp/src/IceBox/ServiceManagerI.cpp

+    // Null observers are ignored
+    if (observer)
+    {
+        addObserver(*std::move(observer));


Uses optional's

constexpr T&& operator*() && noexcept;

bernardnormier · 2024-03-14T21:10:28Z

cpp/src/IceGrid/AdminI.cpp

@@ -129,7 +129,7 @@ namespace

    private:
        string _id;
-        ServerPrxPtr _proxy;
+        optional<ServerPrx> _proxy;


I don't see how _proxy can be null. It's just convenience for the constructor?

We can fix this by using a helper function, and moving the SynchronizationException handling to the helper method.

I will try to fix in a follow up PR.

bernardnormier · 2024-03-14T21:21:06Z

slice/Ice/Locator.ice

@@ -87,7 +87,7 @@ interface LocatorRegistry
    /// Set the adapter endpoints with the locator registry.
    /// @param id The adapter id.
    /// @param proxy The adapter proxy (a dummy direct proxy created by the adapter). The direct proxy contains the
-    /// adapter endpoints.
+    /// adapter endpoints. The proxy is never null.


This is bogus. We pass a null proxy during object adapter deactivation.

bernardnormier · 2024-03-14T21:21:37Z

slice/Ice/Locator.ice

@@ -97,19 +97,19 @@ interface LocatorRegistry
    /// Set the adapter endpoints with the locator registry.
    /// @param adapterId The adapter id.
    /// @param replicaGroupId The replica group id.
-    /// @param p The adapter proxy (a dummy direct proxy created by the adapter). The direct proxy contains the adapter
-    /// endpoints.
+    /// @param proxy The adapter proxy (a dummy direct proxy created by the adapter). The direct proxy contains the adapter


It can be null, see above.

bernardnormier · 2024-03-14T21:21:56Z

slice/Ice/Locator.ice

        throws AdapterNotFoundException, AdapterAlreadyActiveException, InvalidReplicaGroupIdException;

    /// Set the process proxy for a server.
    /// @param id The server id.
-    /// @param proxy The process proxy.
+    /// @param proxy The process proxy. The proxy is never null.


I believe this is correct - this proxy is never null.

bernardnormier · 2024-03-14T21:29:48Z

slice/IceGrid/Admin.ice

@@ -75,7 +75,7 @@ struct AdapterInfo
    /// The id of the adapter.
    string id;

-    /// A dummy direct proxy that contains the adapter endpoints.
+    /// A dummy direct proxy that contains the adapter endpoints. This proxy is never null.


I don't think we can make this statement for struct fields in general. The application can always put a null proxy in a struct field.

Then, if you look at the Database implementation, this field is occasionally null.

Such information would be more valuable on an operation that accepts or returns an AdapterInfo or AdapterInfoSeq. Like (if accurate) "the proxy field of the returned adapter info is never null".

…id implementation (zeroc-ice/ice#1933)

Fix proxy usage and creation in IceGrid implementation

d6d4731

pepone requested review from bernardnormier and externl March 14, 2024 15:41

externl approved these changes Mar 14, 2024

View reviewed changes

Review fixes

8193e17

bernardnormier approved these changes Mar 14, 2024

View reviewed changes

Review fixes

06ff26d

pepone merged commit 0cd3c4a into zeroc-ice:main Mar 15, 2024
24 checks passed

This was referenced Mar 15, 2024

IceGrid cleanup #1940

Merged

Update service & plugin implementations to use non-nullable proxies #1820

Closed

InsertCreativityHere pushed a commit to InsertCreativityHere/compiler-comparison that referenced this pull request Jan 1, 2025

HEAD:(zeroc-ice/ice@0cd3c4a31e) Fix proxy usage and creation in IceGr…

2e6378c

…id implementation (zeroc-ice/ice#1933)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix proxy usage and creation in IceGrid implementation #1933

Fix proxy usage and creation in IceGrid implementation #1933

pepone commented Mar 14, 2024

externl left a comment

externl Mar 14, 2024

pepone Mar 14, 2024

externl Mar 14, 2024

externl Mar 14, 2024

pepone Mar 14, 2024

externl Mar 14, 2024

pepone Mar 14, 2024

externl Mar 14, 2024

pepone Mar 14, 2024

externl Mar 14, 2024

pepone Mar 14, 2024

externl Mar 14, 2024

pepone Mar 14, 2024

bernardnormier left a comment

bernardnormier Mar 14, 2024

bernardnormier Mar 14, 2024

pepone Mar 14, 2024

pepone Mar 15, 2024

bernardnormier Mar 14, 2024

bernardnormier Mar 14, 2024

bernardnormier Mar 14, 2024

bernardnormier Mar 14, 2024

		name == _replicaName ? _database->getReplicaCache().getInternalRegistry()
		: _database->getReplicaCache().getInternalRegistry(),

Fix proxy usage and creation in IceGrid implementation #1933

Fix proxy usage and creation in IceGrid implementation #1933

Conversation

pepone commented Mar 14, 2024

externl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bernardnormier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment