From ff6d9a5f06fd1253d601d88eaf00eaf23f52dd64 Mon Sep 17 00:00:00 2001 From: Bernard Normier Date: Thu, 5 Dec 2024 16:47:48 -0500 Subject: [PATCH 1/5] Fix lints in C++ code (#3238) --- cpp/include/Ice/FactoryTable.h | 14 +++++++------- cpp/include/Ice/StreamHelpers.h | 2 +- cpp/src/Ice/FactoryTable.cpp | 4 ++-- cpp/src/slice2cpp/Gen.cpp | 21 ++++++++++++++++++--- 4 files changed, 28 insertions(+), 13 deletions(-) diff --git a/cpp/include/Ice/FactoryTable.h b/cpp/include/Ice/FactoryTable.h index 10b3ca22547..fab1e7370f2 100644 --- a/cpp/include/Ice/FactoryTable.h +++ b/cpp/include/Ice/FactoryTable.h @@ -21,7 +21,7 @@ namespace IceInternal class ICE_API FactoryTable final { public: - FactoryTable() = default; + FactoryTable() noexcept = default; FactoryTable(const FactoryTable&) = delete; FactoryTable& operator=(const FactoryTable&) = delete; @@ -71,14 +71,14 @@ namespace IceInternal class ICE_API FactoryTableInit { public: - FactoryTableInit(); + FactoryTableInit() noexcept; ~FactoryTableInit(); }; class ICE_API CompactIdInit { public: - CompactIdInit(std::string_view, int); + CompactIdInit(std::string_view, int) noexcept; ~CompactIdInit(); private: @@ -88,27 +88,27 @@ namespace IceInternal template class DefaultUserExceptionFactoryInit { public: - DefaultUserExceptionFactoryInit(std::string_view tId) : typeId(tId) + DefaultUserExceptionFactoryInit(const char* tId) noexcept : typeId(tId) { factoryTable->addExceptionFactory(typeId, defaultUserExceptionFactory); } ~DefaultUserExceptionFactoryInit() { factoryTable->removeExceptionFactory(typeId); } - const std::string_view typeId; + const char* typeId; }; template class DefaultValueFactoryInit { public: - DefaultValueFactoryInit(std::string_view tId) : typeId(tId) + DefaultValueFactoryInit(const char* tId) noexcept : typeId(tId) { factoryTable->addValueFactory(typeId, defaultValueFactory); } ~DefaultValueFactoryInit() { factoryTable->removeValueFactory(typeId); } - const std::string_view typeId; + const char* typeId; }; } diff --git a/cpp/include/Ice/StreamHelpers.h b/cpp/include/Ice/StreamHelpers.h index a8faae2cb6e..0236f3f7972 100644 --- a/cpp/include/Ice/StreamHelpers.h +++ b/cpp/include/Ice/StreamHelpers.h @@ -111,7 +111,7 @@ namespace Ice { IceInternal::Ex::throwMarshalException(__FILE__, __LINE__, "enumerator out of range"); } - v = static_cast(value); + v = static_cast(value); // NOLINT } }; diff --git a/cpp/src/Ice/FactoryTable.cpp b/cpp/src/Ice/FactoryTable.cpp index d68b591d09a..94022c4d69d 100644 --- a/cpp/src/Ice/FactoryTable.cpp +++ b/cpp/src/Ice/FactoryTable.cpp @@ -172,7 +172,7 @@ IceInternal::FactoryTable::removeTypeId(int compactId) // header file that uses non-local exceptions or non-abstract classes. // This ensures that IceInternal::factoryTable is always initialized // before it is used. -IceInternal::FactoryTableInit::FactoryTableInit() +IceInternal::FactoryTableInit::FactoryTableInit() noexcept { if (0 == initCount++) { @@ -190,7 +190,7 @@ IceInternal::FactoryTableInit::~FactoryTableInit() } } -IceInternal::CompactIdInit::CompactIdInit(string_view typeId, int compactId) : _compactId(compactId) +IceInternal::CompactIdInit::CompactIdInit(string_view typeId, int compactId) noexcept : _compactId(compactId) { assert(_compactId >= 0); factoryTable->addTypeId(_compactId, typeId); diff --git a/cpp/src/slice2cpp/Gen.cpp b/cpp/src/slice2cpp/Gen.cpp index 3cc0f13ff19..4ce4e54eb3b 100644 --- a/cpp/src/slice2cpp/Gen.cpp +++ b/cpp/src/slice2cpp/Gen.cpp @@ -1673,7 +1673,7 @@ Slice::Gen::ProxyVisitor::visitOperation(const OperationPtr& p) C << sb; if (lambdaOutParams.size() > 1) { - C << nl << "auto responseCb = [response = ::std::move(response)](" << lambdaT << "&& result)"; + C << nl << "auto responseCb = [response = ::std::move(response)](" << lambdaT << "&& result) mutable"; C << sb; C << nl << "::std::apply(::std::move(response), ::std::move(result));"; C << eb << ";"; @@ -2025,6 +2025,12 @@ Slice::Gen::DataDefVisitor::visitExceptionStart(const ExceptionPtr& p) H.dec(); H << sb; H << eb; + + // We generate a noexcept copy constructor all the time. A C++ exception must have noexcept copy constructor + // but the default constructor is not always noexcept (e.g. if the exception has a string field). + H << sp; + H << nl << "/// Copy constructor."; + H << nl << name << "(const " << name << "&) noexcept = default;"; } H << sp; } @@ -2901,9 +2907,18 @@ Slice::Gen::InterfaceVisitor::visitOperation(const OperationPtr& p) C << nl << "/// \\cond INTERNAL"; C << nl << "void"; C << nl << scope.substr(2); - C << "_iceD_" << name - << "(::Ice::IncomingRequest& request, ::std::function sendResponse)" << isConst; + C << "_iceD_" << name << "("; + C.inc(); + C << nl << "::Ice::IncomingRequest& request," << nl + << "::std::function sendResponse)" << isConst; + if (!amd) + { + // We want to use the same signature for sync and async dispatch functions. There is no performance penalty for + // sync functions since we always move this parameter. + C << " // NOLINT:performance-unnecessary-value-param"; + } + C.dec(); C << sb; C << nl << "_iceCheckMode(" << getUnqualified(operationModeToString(p->mode()), interfaceScope) << ", request.current().mode);"; From a4b3baea5fa3182efa95fe4a7344f9bbcb0f87aa Mon Sep 17 00:00:00 2001 From: Jose Date: Fri, 6 Dec 2024 16:22:08 +0100 Subject: [PATCH 2/5] Add BuildDist alias for Build in C# builds (#3240) --- csharp/msbuild/ice.proj | 2 ++ 1 file changed, 2 insertions(+) diff --git a/csharp/msbuild/ice.proj b/csharp/msbuild/ice.proj index cde7ef681a1..d2a344d38b6 100644 --- a/csharp/msbuild/ice.proj +++ b/csharp/msbuild/ice.proj @@ -58,6 +58,8 @@ Properties="%(Properties)"/> + + Date: Fri, 6 Dec 2024 16:25:54 +0100 Subject: [PATCH 3/5] DataStorm comments and bug fixes (#3239) --- cpp/src/DataStorm/CallbackExecutor.h | 2 -- cpp/src/DataStorm/Contract.ice | 21 ++++++++++++++++- cpp/src/DataStorm/NodeI.cpp | 4 ++-- cpp/src/DataStorm/NodeSessionI.cpp | 1 - cpp/src/DataStorm/SessionI.cpp | 19 +++++++-------- cpp/src/DataStorm/SessionI.h | 35 +++++++++++++++++++++------- cpp/src/DataStorm/TopicI.cpp | 12 +++++----- cpp/src/DataStorm/TopicI.h | 14 +++++++++-- 8 files changed, 76 insertions(+), 32 deletions(-) diff --git a/cpp/src/DataStorm/CallbackExecutor.h b/cpp/src/DataStorm/CallbackExecutor.h index 9385a19669d..51d04176232 100644 --- a/cpp/src/DataStorm/CallbackExecutor.h +++ b/cpp/src/DataStorm/CallbackExecutor.h @@ -14,8 +14,6 @@ namespace DataStormI { - class DataElementI; - class CallbackExecutor final { public: diff --git a/cpp/src/DataStorm/Contract.ice b/cpp/src/DataStorm/Contract.ice index 2e6bbdbc226..03fcd613458 100644 --- a/cpp/src/DataStorm/Contract.ice +++ b/cpp/src/DataStorm/Contract.ice @@ -236,6 +236,12 @@ module DataStormContract } sequence ElementSpecAckSeq; + /// The base interface for publisher and subscriber sessions. + /// + /// This interface enables nodes to exchange topic and element information, as well as data samples. + /// + /// @see PublisherSession + /// @see SubscriberSession interface Session { /// Announces new and existing topics to the peer. @@ -254,7 +260,7 @@ module DataStormContract /// Attaches a local topic to a remote topic when a session receives a topic announcement from a peer. /// - /// This method is called if the session is interested in the announced topic, which occurs when: + /// This operation is called if the session is interested in the announced topic, which occurs when: /// /// - The session has a reader for a topic that the peer has a writer for, or /// - The session has a writer for a topic that the peer has a reader for. @@ -262,6 +268,11 @@ module DataStormContract /// @param topic The TopicSpec object describing the topic being attached to the remote topic. void attachTopic(TopicSpec topic); + /// Detaches a topic from the session. + /// + /// This operation is called by the topic on listener sessions when the topic is being destroyed. + /// + /// @param topic The ID of the topic to detach. void detachTopic(long topic); void attachTags(long topic, ElementInfoSeq tags, bool initialize); @@ -289,13 +300,21 @@ module DataStormContract void initSamples(long topic, DataSamplesSeq samples); + /// Notifies the peer that the session is being disconnected. + /// + /// This operation is called by the DataStorm node during shutdown to inform established sessions of the disconnection. + /// + /// For sessions established through a relay node, this operation is invoked by the relay node if the connection + /// between the relay node and the target node is lost. void disconnected(); } + /// The PublisherSession servant is hosted by the publisher node and is accessed by the subscriber node. interface PublisherSession extends Session { } + /// The SubscriberSession servant is hosted by the subscriber node and is accessed by the publisher node. interface SubscriberSession extends Session { /// Queue a sample with the subscribers of the topic element. diff --git a/cpp/src/DataStorm/NodeI.cpp b/cpp/src/DataStorm/NodeI.cpp index 09dd2d1a8d8..6cb143b11c5 100644 --- a/cpp/src/DataStorm/NodeI.cpp +++ b/cpp/src/DataStorm/NodeI.cpp @@ -199,7 +199,7 @@ NodeI::createSession( // Must be called before connected s->confirmCreateSessionAsync( self->_proxy, - session->getProxy(), + Ice::uncheckedCast(session->getProxy()), nullptr, [self, subscriber, session](auto ex) { self->removePublisherSession(*subscriber, session, ex); }); @@ -330,7 +330,7 @@ NodeI::createPublisherSession( { p->createSessionAsync( self->_proxy, - session->getProxy(), + Ice::uncheckedCast(session->getProxy()), false, nullptr, [=](exception_ptr ex) { self->removeSubscriberSession(publisher, session, ex); }); diff --git a/cpp/src/DataStorm/NodeSessionI.cpp b/cpp/src/DataStorm/NodeSessionI.cpp index 863d6ff6348..796cc4bcb31 100644 --- a/cpp/src/DataStorm/NodeSessionI.cpp +++ b/cpp/src/DataStorm/NodeSessionI.cpp @@ -108,7 +108,6 @@ namespace if (node->ice_getEndpoints().empty() && node->ice_getAdapterId().empty()) { shared_ptr nodeSession = _nodeSessionManager->createOrGet(node, current.con, false); - assert(nodeSession); node = nodeSession->getPublicNode(); if (session) { diff --git a/cpp/src/DataStorm/SessionI.cpp b/cpp/src/DataStorm/SessionI.cpp index 7adac8ea1a5..64d0c8a93a4 100644 --- a/cpp/src/DataStorm/SessionI.cpp +++ b/cpp/src/DataStorm/SessionI.cpp @@ -39,22 +39,21 @@ namespace } SessionI::SessionI(shared_ptr instance, shared_ptr parent, NodePrx node, SessionPrx proxy) - : _instance(std::move(instance)), - _traceLevels(_instance->getTraceLevels()), - _parent(std::move(parent)), - _proxy(std::move(proxy)), - _node(std::move(node)), - _destroyed(false), - _sessionInstanceId(0), - _retryCount(0) + : _instance{std::move(instance)}, + _traceLevels{_instance->getTraceLevels()}, + _parent{std::move(parent)}, + _proxy{std::move(proxy)}, + _id{Ice::identityToString(_proxy->ice_getIdentity())}, + _node{std::move(node)}, + _destroyed{false}, + _sessionInstanceId{0}, + _retryCount{0} { } void SessionI::init() { - _id = Ice::identityToString(_proxy->ice_getIdentity()); - // Even though the node register a default servant for sessions, we still need to register the session servant // explicitly here to ensure collocation works. The default servant from the node is used for facet calls. _instance->getObjectAdapter()->add( diff --git a/cpp/src/DataStorm/SessionI.h b/cpp/src/DataStorm/SessionI.h index 52a86ffbd7d..c60418caf61 100644 --- a/cpp/src/DataStorm/SessionI.h +++ b/cpp/src/DataStorm/SessionI.h @@ -150,7 +150,7 @@ namespace DataStormI auto p = _elements.find(id); if (p != _elements.end()) { - ElementSubscribers tmp(std::move(p->second)); + ElementSubscribers tmp{std::move(p->second)}; _elements.erase(p); return tmp; } @@ -285,10 +285,7 @@ namespace DataStormI std::optional getSession() const; bool checkSession(); - template std::optional getProxy() const - { - return Ice::uncheckedCast(_proxy); - } + DataStormContract::SessionPrx getProxy() const { return _proxy; } DataStormContract::NodePrx getNode() const; void setNode(DataStormContract::NodePrx); @@ -389,21 +386,43 @@ namespace DataStormI const std::shared_ptr _instance; std::shared_ptr _traceLevels; mutable std::mutex _mutex; + + // The parent node that created this session. std::shared_ptr _parent; - std::string _id; + + // The proxy representing this session instance. DataStormContract::SessionPrx _proxy; + + // The stringified identity of the session. + std::string _id; + + // The proxy to the peer node that established the session. DataStormContract::NodePrx _node; + + // Indicates whether the session has been destroyed. bool _destroyed; + + // The instance ID of the session, incremented each time the session is reconnected. int _sessionInstanceId; + + // The number of attempts made to reconnect the session. int _retryCount; + + // A retry task, scheduled if an attempt to reconnect the session is underway; nullptr if no retry is scheduled. IceInternal::TimerTaskPtr _retryTask; - // Keeps track of the topics that this session is subscribed to. The key represents the topic ID in the remote - // node. The TopicSubscribers object contains the subscribers for the remote topic. + // Tracks the topics this session is subscribed to. + // - Key: The topic ID on the remote node. + // - Value: A `TopicSubscribers` object containing the subscribers for the remote topic. std::map _topics; + + // The lock of the topic being processed by the callback function. See runWithTopics. std::unique_lock* _topicLock; + // The proxy to the peer session, or `std::nullopt` if the session is disconnected. std::optional _session; + + // The connection to the peer node, or `nullptr` if the session is disconnected. Ice::ConnectionPtr _connection; }; diff --git a/cpp/src/DataStorm/TopicI.cpp b/cpp/src/DataStorm/TopicI.cpp index 3584eb3e213..2905f2ff96c 100644 --- a/cpp/src/DataStorm/TopicI.cpp +++ b/cpp/src/DataStorm/TopicI.cpp @@ -335,7 +335,7 @@ TopicI::getElementSpecs(int64_t topicId, const ElementInfoSeq& infos, const shar } void -TopicI::attach(int64_t id, shared_ptr session, SessionPrx peerSession) +TopicI::attach(int64_t topicId, shared_ptr session, SessionPrx peerSession) { auto p = _listeners.find(session); if (p == _listeners.end()) @@ -343,19 +343,19 @@ TopicI::attach(int64_t id, shared_ptr session, SessionPrx peerSession) p = _listeners.emplace(std::move(session), Listener(std::move(peerSession))).first; } - if (p->second.topics.insert(id).second) + if (p->second.topics.insert(topicId).second) { - p->first->subscribe(id, this); + p->first->subscribe(topicId, this); } } void -TopicI::detach(int64_t id, const shared_ptr& session) +TopicI::detach(int64_t topicId, const shared_ptr& session) { auto p = _listeners.find(session); - if (p != _listeners.end() && p->second.topics.erase(id)) + if (p != _listeners.end() && p->second.topics.erase(topicId)) { - session->unsubscribe(id, this); + session->unsubscribe(topicId, this); if (p->second.topics.empty()) { _listeners.erase(p); diff --git a/cpp/src/DataStorm/TopicI.h b/cpp/src/DataStorm/TopicI.h index ea15e1d2991..743f0f70abc 100644 --- a/cpp/src/DataStorm/TopicI.h +++ b/cpp/src/DataStorm/TopicI.h @@ -61,8 +61,18 @@ namespace DataStormI const DataStormContract::ElementInfoSeq& infos, const std::shared_ptr& session); - void attach(std::int64_t, std::shared_ptr, DataStormContract::SessionPrx); - void detach(std::int64_t, const std::shared_ptr&); + /// Attach this topic with the remote topic with the given topic ID. + /// + /// @param topicId The remote topic ID to attach with. + /// @param session The session servant in the current node. + /// @param peerSession The peer session proxy. + void attach(std::int64_t topicId, std::shared_ptr session, DataStormContract::SessionPrx peerSession); + + /// Detach this topic from the remote topic with the given topic ID. + /// + /// @param topicId The remote topic ID to detach from. + /// @param session The session servant in the current node. + void detach(std::int64_t topicId, const std::shared_ptr& session); DataStormContract::ElementSpecAckSeq attachElements( std::int64_t, From a0015825352244409f8199b49d38c0885edddb87 Mon Sep 17 00:00:00 2001 From: Bernard Normier Date: Fri, 6 Dec 2024 16:51:01 -0500 Subject: [PATCH 4/5] Consolidate demangle helper function in C++ (#3241) --- .vscode/cspell.json | 8 +++++++ .vscode/settings.json | 8 ------- Package.swift | 1 + cpp/include/DataStorm/InternalT.h | 3 ++- cpp/include/Ice/Demangle.h | 17 ++++++++++++++ cpp/include/Ice/Ice.h | 3 ++- cpp/src/Ice/Demangle.cpp | 25 +++++++++++++++++++++ cpp/src/Ice/Exception.cpp | 14 ++---------- cpp/src/Ice/OutgoingResponse.cpp | 20 +---------------- cpp/src/Ice/msbuild/ice/ice.vcxproj | 1 + cpp/src/Ice/msbuild/ice/ice.vcxproj.filters | 3 +++ cpp/src/IceUtil/Makefile.mk | 1 + cpp/test/include/TestHelper.h | 3 ++- swift/src/IceImpl/Convert.mm | 19 ++-------------- 14 files changed, 67 insertions(+), 59 deletions(-) create mode 100644 cpp/include/Ice/Demangle.h create mode 100644 cpp/src/Ice/Demangle.cpp diff --git a/.vscode/cspell.json b/.vscode/cspell.json index 9d96f79179f..fe57db765dc 100644 --- a/.vscode/cspell.json +++ b/.vscode/cspell.json @@ -5,10 +5,13 @@ "apos", "autoreleasepool", "Blobject", + "Browsable", "cacerts", "datagram", "datagrams", "decoratee", + "demangled", + "demangles", "DTLS", "finalizer", "finalizers", @@ -16,22 +19,27 @@ "icegridadmin", "icegridnode", "icestorm", + "istr", "jgoodies", "LMDB", "nohup", "nullopt", "oneway", + "ostr", "PKCS", "Postamble", "reloadable", "RFCOMM", + "soversion", "Stringifier", "Syscall", "Truststore", "twoway", "unmarshal", "unmarshaling", + "upcall", "upcalls", + "unregisters", "zeroc" ], "flagWords": [ diff --git a/.vscode/settings.json b/.vscode/settings.json index 1113b113535..aab74c7ec21 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -10,14 +10,6 @@ "Make.*": "makefile" }, "C_Cpp.default.cppStandard": "c++20", - "cSpell.words": [ - "Browsable", - "istr", - "ostr", - "soversion", - "unregisters", - "Upcall" - ], "[java]": { "editor.defaultFormatter": "josevseb.google-java-format-for-vs-code" }, diff --git a/Package.swift b/Package.swift index e14b7870f5f..7a8cdf4aa1f 100644 --- a/Package.swift +++ b/Package.swift @@ -6,6 +6,7 @@ import PackageDescription let iceUtilSources: [String] = [ "src/Ice/ConsoleUtil.cpp", "src/Ice/CtrlCHandler.cpp", + "src/Ice/Demangle.cpp", "src/Ice/Exception.cpp", "src/Ice/FileUtil.cpp", "src/Ice/LocalException.cpp", diff --git a/cpp/include/DataStorm/InternalT.h b/cpp/include/DataStorm/InternalT.h index a0c2936d752..f176e498210 100644 --- a/cpp/include/DataStorm/InternalT.h +++ b/cpp/include/DataStorm/InternalT.h @@ -5,6 +5,7 @@ #pragma once #include "Config.h" +#include "Ice/Demangle.h" #include "Ice/Ice.h" #include "InternalI.h" #include "Types.h" @@ -91,7 +92,7 @@ namespace DataStormI static std::string toString(const T& value) { std::ostringstream os; - os << typeid(value).name() << '(' << &value << ')'; + os << IceInternal::demangle(typeid(value).name()) << '(' << &value << ')'; return os.str(); } }; diff --git a/cpp/include/Ice/Demangle.h b/cpp/include/Ice/Demangle.h new file mode 100644 index 00000000000..eefef51c0e4 --- /dev/null +++ b/cpp/include/Ice/Demangle.h @@ -0,0 +1,17 @@ +// Copyright (c) ZeroC, Inc. + +#ifndef ICE_DEMANGLE_H +#define ICE_DEMANGLE_H + +#include "Config.h" +#include + +namespace IceInternal +{ + /// Demangles a C++ type type. + /// @param name The possibly mangled type name, as returned by typeid().name(). + /// @return The demangled name. + ICE_API std::string demangle(const char* name); +} + +#endif diff --git a/cpp/include/Ice/Ice.h b/cpp/include/Ice/Ice.h index bf144b33298..e8a0575fb5b 100644 --- a/cpp/include/Ice/Ice.h +++ b/cpp/include/Ice/Ice.h @@ -22,6 +22,8 @@ # include "Communicator.h" # include "Connection.h" +# include "CtrlCHandler.h" +# include "Demangle.h" # include "ImplicitContext.h" # include "Initialize.h" # include "Instrumentation.h" @@ -48,7 +50,6 @@ # include "VersionFunctions.h" // Generated header files: -# include "CtrlCHandler.h" # include "Ice/EndpointTypes.h" # include "Ice/Locator.h" # include "Ice/Metrics.h" diff --git a/cpp/src/Ice/Demangle.cpp b/cpp/src/Ice/Demangle.cpp new file mode 100644 index 00000000000..b5124ba0512 --- /dev/null +++ b/cpp/src/Ice/Demangle.cpp @@ -0,0 +1,25 @@ +// Copyright (c) ZeroC, Inc. + +#include "Ice/Demangle.h" + +#if defined(__GNUC__) || defined(__clang__) +# include +#endif + +using namespace std; + +string +IceInternal::demangle(const char* name) +{ +#if defined(__GNUC__) || defined(__clang__) + int status; + char* demangled = abi::__cxa_demangle(name, nullptr, nullptr, &status); + if (status == 0) // success + { + string result{demangled}; + std::free(demangled); + return result; + } +#endif + return name; // keep the original name +} diff --git a/cpp/src/Ice/Exception.cpp b/cpp/src/Ice/Exception.cpp index 94e07435da2..182ae31de10 100644 --- a/cpp/src/Ice/Exception.cpp +++ b/cpp/src/Ice/Exception.cpp @@ -20,6 +20,7 @@ #include "Ice/Exception.h" #include "Ice/Config.h" +#include "Ice/Demangle.h" #include "Ice/StringUtil.h" #ifdef _WIN32 @@ -39,7 +40,6 @@ # include # if BACKTRACE_SUPPORTED && BACKTRACE_SUPPORTS_THREADS # include -# include # else // It's available but we can't use it - shouldn't happen # undef ICE_LIBBACKTRACE @@ -47,7 +47,6 @@ # endif # if !defined(__FreeBSD__) -# include # include # include # define ICE_BACKTRACE @@ -204,16 +203,7 @@ namespace if (function) { - char* demangledFunction = abi::__cxa_demangle(function, 0, 0, 0); - if (demangledFunction) - { - os << demangledFunction; - free(demangledFunction); - } - else - { - os << function; - } + os << IceInternal::demangle(function); if (filename && lineno > 0) { diff --git a/cpp/src/Ice/OutgoingResponse.cpp b/cpp/src/Ice/OutgoingResponse.cpp index 67ea69194e8..d1ea779d558 100644 --- a/cpp/src/Ice/OutgoingResponse.cpp +++ b/cpp/src/Ice/OutgoingResponse.cpp @@ -3,16 +3,13 @@ // #include "Ice/OutgoingResponse.h" +#include "Ice/Demangle.h" #include "Ice/LocalExceptions.h" #include "Ice/ObjectAdapter.h" #include "Ice/UserException.h" #include "Protocol.h" #include "RequestFailedMessage.h" -#if defined(__GNUC__) || defined(__clang__) -# include -#endif - #include using namespace std; @@ -36,21 +33,6 @@ namespace return os.str(); } - inline string demangle(const char* name) - { -#if defined(__GNUC__) || defined(__clang__) - int status; - char* demangled = abi::__cxa_demangle(name, nullptr, nullptr, &status); - if (status == 0) // success - { - string result{demangled}; - std::free(demangled); - return result; - } -#endif - return name; // keep the original name - } - // The "core" implementation of makeOutgoingResponse for exceptions. Note that it can throw an exception. OutgoingResponse makeOutgoingResponseCore(std::exception_ptr exc, const Current& current) { diff --git a/cpp/src/Ice/msbuild/ice/ice.vcxproj b/cpp/src/Ice/msbuild/ice/ice.vcxproj index 9d1ea5cf2ea..c9ce13df0dd 100644 --- a/cpp/src/Ice/msbuild/ice/ice.vcxproj +++ b/cpp/src/Ice/msbuild/ice/ice.vcxproj @@ -167,6 +167,7 @@ + diff --git a/cpp/src/Ice/msbuild/ice/ice.vcxproj.filters b/cpp/src/Ice/msbuild/ice/ice.vcxproj.filters index 6473fb159bd..519cda74b18 100644 --- a/cpp/src/Ice/msbuild/ice/ice.vcxproj.filters +++ b/cpp/src/Ice/msbuild/ice/ice.vcxproj.filters @@ -502,6 +502,9 @@ Source Files + + Source Files + Source Files diff --git a/cpp/src/IceUtil/Makefile.mk b/cpp/src/IceUtil/Makefile.mk index 5ea885b0930..21323055bd1 100644 --- a/cpp/src/IceUtil/Makefile.mk +++ b/cpp/src/IceUtil/Makefile.mk @@ -11,6 +11,7 @@ IceUtil_cppflags := $(if $(filter yes,$(libbacktrace)),-DICE_LIBBACKTRACE IceUtil_extra_sources := src/Ice/ConsoleUtil.cpp \ src/Ice/CtrlCHandler.cpp \ + src/Ice/Demangle.cpp \ src/Ice/Exception.cpp \ src/Ice/FileUtil.cpp \ src/Ice/LocalException.cpp \ diff --git a/cpp/test/include/TestHelper.h b/cpp/test/include/TestHelper.h index 89cb52e2765..43a31ee30df 100644 --- a/cpp/test/include/TestHelper.h +++ b/cpp/test/include/TestHelper.h @@ -8,6 +8,7 @@ #include "Ice/CommunicatorF.h" #include "Ice/Config.h" #include "Ice/CtrlCHandler.h" +#include "Ice/Demangle.h" #include "Ice/Initialize.h" #include "Ice/LocalExceptions.h" #include "Ice/Logger.h" @@ -157,7 +158,7 @@ namespace Test } catch (const std::exception& ex) { - std::cerr << "error: " << typeid(ex).name() << ' ' << ex.what() << std::endl; + std::cerr << "error: " << IceInternal::demangle(typeid(ex).name()) << ' ' << ex.what() << std::endl; status = 1; } return status; diff --git a/swift/src/IceImpl/Convert.mm b/swift/src/IceImpl/Convert.mm index 621598e0ce3..cc62885bf2c 100644 --- a/swift/src/IceImpl/Convert.mm +++ b/swift/src/IceImpl/Convert.mm @@ -4,7 +4,6 @@ #import "include/LocalExceptionFactory.h" #include -#include #include #include @@ -88,22 +87,8 @@ } catch (const std::exception& e) { - int status = 0; - const char* mangled = typeid(e).name(); - char* demangled = abi::__cxa_demangle(mangled, nullptr, nullptr, &status); - - NSError* error = nullptr; - if (status == 0) // success - { - error = [factory cxxException:toNSString(demangled) message:toNSString(e.what())]; - std::free(demangled); - } - else - { - error = [factory cxxException:toNSString(mangled) message:toNSString(e.what())]; - assert(demangled == nullptr); - } - return error; + std::string demangled = IceInternal::demangle(typeid(e).name()); + return [factory cxxException:toNSString(demangled) message:toNSString(e.what())]; } catch (...) { From 449081a1b0010887c626f3004741a3867340fb16 Mon Sep 17 00:00:00 2001 From: Bernard Normier Date: Mon, 9 Dec 2024 11:25:55 -0500 Subject: [PATCH 5/5] Fix ice_getConnectionAsync (#3242) --- cpp/src/Ice/OutgoingAsync.cpp | 12 ++---- cpp/src/Ice/ProxyAsync.cpp | 27 +++++++++--- cpp/test/Ice/ami/AllTests.cpp | 15 +++++++ csharp/src/Ice/Internal/OutgoingAsync.cs | 42 +++++++++++-------- csharp/src/Ice/Proxy.cs | 18 +------- csharp/test/Ice/ami/AllTests.cs | 13 +++++- .../java/com/zeroc/Ice/OutgoingAsync.java | 4 +- .../java/com/zeroc/Ice/ProxyFlushBatch.java | 6 ++- .../com/zeroc/Ice/ProxyGetConnection.java | 6 ++- .../java/com/zeroc/Ice/ProxyIceInvoke.java | 6 +-- .../com/zeroc/Ice/ProxyOutgoingAsyncBase.java | 6 +-- .../src/main/java/test/Ice/ami/AllTests.java | 6 +++ python/test/Ice/ami/AllTests.py | 6 +-- 13 files changed, 100 insertions(+), 67 deletions(-) diff --git a/cpp/src/Ice/OutgoingAsync.cpp b/cpp/src/Ice/OutgoingAsync.cpp index 92d76b2f60c..e828fc32c18 100644 --- a/cpp/src/Ice/OutgoingAsync.cpp +++ b/cpp/src/Ice/OutgoingAsync.cpp @@ -494,10 +494,8 @@ ProxyOutgoingAsyncBase::invokeImpl(bool userThread) } catch (const Exception&) { - // - // If called from the user thread we re-throw, the exception - // will be catch by the caller and abort(ex) will be called. - // + // If called from the user thread we re-throw, the exception will be caught by the caller and handled using + // abort. if (userThread) { throw; @@ -1064,11 +1062,7 @@ OutgoingAsync::invoke(string_view operation) return; } - // - // NOTE: invokeImpl doesn't throw so this can be called from the - // try block with the catch block calling abort(ex) in case of an - // exception. - // + // invokeImpl can throw. The exception should be handled by calling abort (in the caller). invokeImpl(true); // userThread = true } diff --git a/cpp/src/Ice/ProxyAsync.cpp b/cpp/src/Ice/ProxyAsync.cpp index c1f896ec13a..59700e15838 100644 --- a/cpp/src/Ice/ProxyAsync.cpp +++ b/cpp/src/Ice/ProxyAsync.cpp @@ -261,10 +261,18 @@ ProxyFlushBatchAsync::invoke(string_view operation) "cannot send request using protocol version " + Ice::protocolVersionToString(_proxy._getReference()->getProtocol())}; } - _observer.attach(_proxy, operation, noExplicitContext); - bool compress; // Ignore for proxy flushBatchRequests - _batchRequestNum = _proxy._getReference()->getBatchRequestQueue()->swap(&_os, compress); - invokeImpl(true); // userThread = true + + try + { + _observer.attach(_proxy, operation, noExplicitContext); + bool compress; // Ignore for proxy flushBatchRequests + _batchRequestNum = _proxy._getReference()->getBatchRequestQueue()->swap(&_os, compress); + invokeImpl(true); // userThread = true + } + catch (const Exception&) + { + abort(current_exception()); + } } ProxyGetConnection::ProxyGetConnection(ObjectPrx proxy) : ProxyOutgoingAsyncBase(std::move(proxy)) {} @@ -299,8 +307,15 @@ ProxyGetConnection::getConnection() const void ProxyGetConnection::invoke(string_view operation) { - _observer.attach(_proxy, operation, noExplicitContext); - invokeImpl(true); // userThread = true + try + { + _observer.attach(_proxy, operation, noExplicitContext); + invokeImpl(true); // userThread = true + } + catch (const Exception&) + { + abort(current_exception()); + } } bool diff --git a/cpp/test/Ice/ami/AllTests.cpp b/cpp/test/Ice/ami/AllTests.cpp index 874559667cc..6073ef06cc0 100644 --- a/cpp/test/Ice/ami/AllTests.cpp +++ b/cpp/test/Ice/ami/AllTests.cpp @@ -444,6 +444,21 @@ allTests(TestHelper* helper, bool collocated) } } + { + promise promise; + indirect->ice_getConnectionAsync( + [&](Ice::ConnectionPtr connection) { promise.set_value(std::move(connection)); }, + [&](exception_ptr ex) { promise.set_exception(ex); }); + try + { + promise.get_future().get(); + test(false); + } + catch (const NoEndpointException&) + { + } + } + { try { diff --git a/csharp/src/Ice/Internal/OutgoingAsync.cs b/csharp/src/Ice/Internal/OutgoingAsync.cs index e547b0059df..323e4ccf798 100644 --- a/csharp/src/Ice/Internal/OutgoingAsync.cs +++ b/csharp/src/Ice/Internal/OutgoingAsync.cs @@ -586,10 +586,8 @@ protected void invokeImpl(bool userThread) } catch (Ice.Exception ex) { - // - // If called from the user thread we re-throw, the exception - // will be catch by the caller and abort() will be called. - // + // If called from the user thread we re-throw, the exception will caught by the caller and handled using + // abort. if (userThread) { throw; @@ -1070,11 +1068,7 @@ protected void invoke(string operation, bool synchronous) return; } - // - // NOTE: invokeImpl doesn't throw so this can be called from the - // try block with the catch block calling abort() in case of an - // exception. - // + // invokeImpl can throw invokeImpl(true); // userThread = true } @@ -1276,11 +1270,18 @@ public void invoke(string operation, bool synchronous) throw new FeatureNotSupportedException( $"Cannot send request using protocol version {proxy_.iceReference().getProtocol()}."); } - synchronous_ = synchronous; - observer_ = ObserverHelper.get(proxy_, operation, null); - // Not used for proxy flush batch requests. - _batchRequestNum = proxy_.iceReference().batchRequestQueue.swap(os_, out _); - invokeImpl(true); // userThread = true + try + { + synchronous_ = synchronous; + observer_ = ObserverHelper.get(proxy_, operation, null); + // Not used for proxy flush batch requests. + _batchRequestNum = proxy_.iceReference().batchRequestQueue.swap(os_, out _); + invokeImpl(true); // userThread = true + } + catch (Ice.Exception ex) + { + abort(ex); + } } private int _batchRequestNum; @@ -1322,9 +1323,16 @@ public Ice.Connection getConnection() public void invoke(string operation, bool synchronous) { - synchronous_ = synchronous; - observer_ = ObserverHelper.get(proxy_, operation, null); - invokeImpl(true); // userThread = true + try + { + synchronous_ = synchronous; + observer_ = ObserverHelper.get(proxy_, operation, null); + invokeImpl(true); // userThread = true + } + catch (Ice.Exception ex) + { + abort(ex); + } } } diff --git a/csharp/src/Ice/Proxy.cs b/csharp/src/Ice/Proxy.cs index c1f4f59015e..467fadb50c2 100644 --- a/csharp/src/Ice/Proxy.cs +++ b/csharp/src/Ice/Proxy.cs @@ -1583,14 +1583,7 @@ public override void handleInvokeResponse(bool ok, OutgoingAsyncBase og) private void iceI_ice_getConnection(OutgoingAsyncCompletionCallback completed, bool synchronous) { var outgoing = new ProxyGetConnection(this, completed); - try - { - outgoing.invoke(_ice_getConnection_name, synchronous); - } - catch (Ice.Exception ex) - { - outgoing.abort(ex); - } + outgoing.invoke(_ice_getConnection_name, synchronous); } /// @@ -1632,14 +1625,7 @@ public Task ice_flushBatchRequestsAsync( private void iceI_ice_flushBatchRequests(OutgoingAsyncCompletionCallback completed, bool synchronous) { var outgoing = new ProxyFlushBatchAsync(this, completed); - try - { - outgoing.invoke(_ice_flushBatchRequests_name, synchronous); - } - catch (Exception ex) - { - outgoing.abort(ex); - } + outgoing.invoke(_ice_flushBatchRequests_name, synchronous); } public System.Threading.Tasks.TaskScheduler ice_scheduler() diff --git a/csharp/test/Ice/ami/AllTests.cs b/csharp/test/Ice/ami/AllTests.cs index 7f14544dd2b..37a2219bf39 100644 --- a/csharp/test/Ice/ami/AllTests.cs +++ b/csharp/test/Ice/ami/AllTests.cs @@ -182,9 +182,20 @@ public static async Task allTestsAsync(global::Test.TestHelper helper, bool coll { Test.TestIntfPrx indirect = Test.TestIntfPrxHelper.uncheckedCast(p.ice_adapterId("dummy")); + Task t = indirect.opAsync(); try { - await indirect.opAsync(); + await t; + test(false); + } + catch (NoEndpointException) + { + } + + t = indirect.ice_getConnectionAsync(); + try + { + await t; test(false); } catch (NoEndpointException) diff --git a/java/src/com.zeroc.ice/src/main/java/com/zeroc/Ice/OutgoingAsync.java b/java/src/com.zeroc.ice/src/main/java/com/zeroc/Ice/OutgoingAsync.java index daa1ccd176e..906070b2762 100644 --- a/java/src/com.zeroc.ice/src/main/java/com/zeroc/Ice/OutgoingAsync.java +++ b/java/src/com.zeroc.ice/src/main/java/com/zeroc/Ice/OutgoingAsync.java @@ -73,9 +73,7 @@ public void invoke( .finishBatchRequest(_os, _proxy, _operation); finished(true, false); } else { - // NOTE: invokeImpl doesn't throw so this can be called from the - // try block with the catch block calling abort() in case of an - // exception. + // invokeImpl can throw; we handle this exception by calling abort invokeImpl(true); // userThread = true } } catch (LocalException ex) { diff --git a/java/src/com.zeroc.ice/src/main/java/com/zeroc/Ice/ProxyFlushBatch.java b/java/src/com.zeroc.ice/src/main/java/com/zeroc/Ice/ProxyFlushBatch.java index d3087457847..7e0fcab5afc 100644 --- a/java/src/com.zeroc.ice/src/main/java/com/zeroc/Ice/ProxyFlushBatch.java +++ b/java/src/com.zeroc.ice/src/main/java/com/zeroc/Ice/ProxyFlushBatch.java @@ -44,7 +44,11 @@ public int invokeCollocated(CollocatedRequestHandler handler) { public void invoke() { Protocol.checkSupportedProtocol( Protocol.getCompatibleProtocol(_proxy._getReference().getProtocol())); - invokeImpl(true); // userThread = true + try { + invokeImpl(true); // userThread = true + } catch (LocalException ex) { + abort(ex); + } } protected int _batchRequestNum; diff --git a/java/src/com.zeroc.ice/src/main/java/com/zeroc/Ice/ProxyGetConnection.java b/java/src/com.zeroc.ice/src/main/java/com/zeroc/Ice/ProxyGetConnection.java index 9445adb1f22..046ce968983 100644 --- a/java/src/com.zeroc.ice/src/main/java/com/zeroc/Ice/ProxyGetConnection.java +++ b/java/src/com.zeroc.ice/src/main/java/com/zeroc/Ice/ProxyGetConnection.java @@ -40,6 +40,10 @@ public int invokeCollocated(CollocatedRequestHandler handler) { } public void invoke() { - invokeImpl(true); // userThread = true + try { + invokeImpl(true); // userThread = true + } catch (LocalException ex) { + abort(ex); + } } } diff --git a/java/src/com.zeroc.ice/src/main/java/com/zeroc/Ice/ProxyIceInvoke.java b/java/src/com.zeroc.ice/src/main/java/com/zeroc/Ice/ProxyIceInvoke.java index 8bfb573f6a6..9a03d9d8d75 100644 --- a/java/src/com.zeroc.ice/src/main/java/com/zeroc/Ice/ProxyIceInvoke.java +++ b/java/src/com.zeroc.ice/src/main/java/com/zeroc/Ice/ProxyIceInvoke.java @@ -31,11 +31,7 @@ public void invoke(byte[] inParams, java.util.Map ctx) { .finishBatchRequest(_os, _proxy, _operation); finished(true, false); } else { - // - // NOTE: invokeImpl doesn't throw so this can be called from the - // try block with the catch block calling abort() in case of an - // exception. - // + // invokeImpl can throw and we handle the exception with abort. invokeImpl(true); // userThread = true } } catch (LocalException ex) { diff --git a/java/src/com.zeroc.ice/src/main/java/com/zeroc/Ice/ProxyOutgoingAsyncBase.java b/java/src/com.zeroc.ice/src/main/java/com/zeroc/Ice/ProxyOutgoingAsyncBase.java index 7762e2eca24..7eedce77e10 100644 --- a/java/src/com.zeroc.ice/src/main/java/com/zeroc/Ice/ProxyOutgoingAsyncBase.java +++ b/java/src/com.zeroc.ice/src/main/java/com/zeroc/Ice/ProxyOutgoingAsyncBase.java @@ -248,10 +248,8 @@ protected void invokeImpl(boolean userThread) { } } } catch (LocalException ex) { - // - // If called from the user thread we re-throw, the exception - // will be catch by the caller and abort() will be called. - // + // If called from the user thread we re-throw: the exception + // will be caught by the caller and handled using abort(). if (userThread) { throw ex; } else if (finished(ex)) // No retries, we're done diff --git a/java/test/src/main/java/test/Ice/ami/AllTests.java b/java/test/src/main/java/test/Ice/ami/AllTests.java index 8baee79a237..ba8342463f2 100644 --- a/java/test/src/main/java/test/Ice/ami/AllTests.java +++ b/java/test/src/main/java/test/Ice/ami/AllTests.java @@ -202,6 +202,12 @@ public static void allTests(test.TestHelper helper, boolean collocated) { test(ex != null && ex instanceof com.zeroc.Ice.NoEndpointException); }); + indirect.ice_getConnectionAsync() + .whenComplete( + (result, ex) -> { + test(ex != null && ex instanceof com.zeroc.Ice.NoEndpointException); + }); + // // Check that CommunicatorDestroyedException is raised directly. // diff --git a/python/test/Ice/ami/AllTests.py b/python/test/Ice/ami/AllTests.py index 9abf653c41c..df726264706 100644 --- a/python/test/Ice/ami/AllTests.py +++ b/python/test/Ice/ami/AllTests.py @@ -700,10 +700,8 @@ def allTestsFuture(helper, communicator, collocated): cb.check() if not collocated: - try: - i.ice_getConnectionAsync() - except Ice.NoEndpointException: - pass + i.ice_getConnectionAsync().add_done_callback(cb.ex) + cb.check() i.opAsync().add_done_callback(cb.ex) cb.check()