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.

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

Jump to bottom

Simplify request handler - proxy relationship #1776

Merged
merged 4 commits into from
Feb 9, 2024
Merged

Simplify request handler - proxy relationship #1776

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
145a4f1
Removed request handler dependency on proxies
bentoi Feb 8, 2024
2f2b919
Merge remote-tracking branch 'origin/main' into request-handler1
bernardnormier Feb 9, 2024
5110db9
Merge
bernardnormier Feb 9, 2024
111c9e5
Remove more code
bernardnormier Feb 9, 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
2 changes: 1 addition & 1 deletion cpp/include/Ice/Proxy.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1182,7 +1182,7 @@ class ICE_API ObjectPrx : public Proxy<ObjectPrx>, public ::std::enable_shared_f
::IceInternal::RequestHandlerPtr _getRequestHandler();
::IceInternal::BatchRequestQueuePtr _getBatchRequestQueue() const { return _batchRequestQueue; }
::IceInternal::RequestHandlerPtr _setRequestHandler(const ::IceInternal::RequestHandlerPtr&);
void _updateRequestHandler(const ::IceInternal::RequestHandlerPtr&, const ::IceInternal::RequestHandlerPtr&);
void _clearRequestHandler(const ::IceInternal::RequestHandlerPtr&);

int _hash() const;

Expand Down
6 changes: 0 additions & 6 deletions cpp/src/Ice/CollocatedRequestHandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,12 +77,6 @@ CollocatedRequestHandler::~CollocatedRequestHandler()
{
}

RequestHandlerPtr
CollocatedRequestHandler::update(const RequestHandlerPtr& previousHandler, const RequestHandlerPtr& newHandler)
{
return previousHandler.get() == this ? newHandler : shared_from_this();
}

AsyncStatus
CollocatedRequestHandler::sendAsyncRequest(const ProxyOutgoingAsyncBasePtr& outAsync)
{
Expand Down
2 changes: 0 additions & 2 deletions cpp/src/Ice/CollocatedRequestHandler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,6 @@ class CollocatedRequestHandler : public RequestHandler, public ResponseHandler
CollocatedRequestHandler(const ReferencePtr&, const Ice::ObjectAdapterPtr&);
virtual ~CollocatedRequestHandler();

virtual RequestHandlerPtr update(const RequestHandlerPtr&, const RequestHandlerPtr&);

virtual AsyncStatus sendAsyncRequest(const ProxyOutgoingAsyncBasePtr&);

virtual void asyncRequestCanceled(const OutgoingAsyncBasePtr&, std::exception_ptr);
Expand Down
40 changes: 5 additions & 35 deletions cpp/src/Ice/ConnectRequestHandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,35 +13,25 @@
#include <Ice/Protocol.h>
#include <Ice/Properties.h>
#include <Ice/ThreadPool.h>
#include <Ice/ProxyFactory.h>

using namespace std;
using namespace IceInternal;

ConnectRequestHandler::ConnectRequestHandler(const ReferencePtr& ref, const Ice::ObjectPrxPtr& proxy) :
ConnectRequestHandler::ConnectRequestHandler(const ReferencePtr& ref) :
RequestHandler(ref),
_proxy(proxy),
_initialized(false),
_flushing(false)
{
}

RequestHandlerPtr
ConnectRequestHandler::connect(const Ice::ObjectPrxPtr& proxy)
ConnectRequestHandler::connect()
{
unique_lock lock(_mutex);
if(!initialized(lock))
{
_proxies.insert(proxy);
}
return _requestHandler ? _requestHandler : shared_from_this();
Copy link
Member

Choose a reason for hiding this comment

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

Seems that we never set _requestHandler

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right: I remove this _requestHandler and even the enclosing connect function.

}

RequestHandlerPtr
ConnectRequestHandler::update(const RequestHandlerPtr& previousHandler, const RequestHandlerPtr& newHandler)
{
return previousHandler.get() == this ? newHandler : shared_from_this();
}

AsyncStatus
ConnectRequestHandler::sendAsyncRequest(const ProxyOutgoingAsyncBasePtr& out)
{
Expand Down Expand Up @@ -144,16 +134,15 @@ ConnectRequestHandler::setConnection(const Ice::ConnectionIPtr& connection, bool
}

//
// If this proxy is for a non-local object, and we are using a router, then
// add this proxy to the router info object.
// If we are using a router, add this proxy to the router info object.
//
RouterInfoPtr ri = _reference->getRouterInfo();

if (ri)
{
auto self = shared_from_this();
if (!ri->addProxyAsync(
_proxy,
_reference->getInstance()->proxyFactory()->referenceToProxy(_reference),
[self] { self->addedProxy(); },
[self](exception_ptr ex) { self->setException(ex); }))
{
Expand Down Expand Up @@ -204,8 +193,6 @@ ConnectRequestHandler::setException(exception_ptr ex)
{
lock_guard lock(_mutex);
_flushing = false;
_proxies.clear();
_proxy = 0; // Break cyclic reference count.
_conditionVariable.notify_all();
}
}
Expand Down Expand Up @@ -302,21 +289,6 @@ ConnectRequestHandler::flushRequests()
_requests.pop_front();
}

//
// If we aren't caching the connection, don't bother creating a
// connection request handler. Otherwise, update the proxies
// request handler to use the more efficient connection request
// handler.
//
if(_reference->getCacheConnection() && !exception)
{
_requestHandler = make_shared<ConnectionRequestHandler>(_reference, _connection, _compress);
for(set<Ice::ObjectPrxPtr>::const_iterator p = _proxies.begin(); p != _proxies.end(); ++p)
{
(*p)->_updateRequestHandler(shared_from_this(), _requestHandler);
}
}

{
lock_guard lock(_mutex);
assert(!_initialized);
Expand All @@ -330,8 +302,6 @@ ConnectRequestHandler::flushRequests()
//
_reference->getInstance()->requestHandlerFactory()->removeRequestHandler(_reference, shared_from_this());

_proxies.clear();
_proxy = nullptr; // Break cyclic reference count.
_conditionVariable.notify_all();
}
}
8 changes: 2 additions & 6 deletions cpp/src/Ice/ConnectRequestHandler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,9 @@ class ConnectRequestHandler final :
{
public:

ConnectRequestHandler(const ReferencePtr&, const Ice::ObjectPrxPtr&);
ConnectRequestHandler(const ReferencePtr&);

RequestHandlerPtr connect(const Ice::ObjectPrxPtr&);
virtual RequestHandlerPtr update(const RequestHandlerPtr&, const RequestHandlerPtr&);
RequestHandlerPtr connect();

virtual AsyncStatus sendAsyncRequest(const ProxyOutgoingAsyncBasePtr&);

Expand All @@ -48,9 +47,6 @@ class ConnectRequestHandler final :
bool initialized(std::unique_lock<std::mutex>&);
void flushRequests();

Ice::ObjectPrxPtr _proxy;
std::set<Ice::ObjectPrxPtr> _proxies;

Ice::ConnectionIPtr _connection;
bool _compress;
std::exception_ptr _exception;
Expand Down
33 changes: 3 additions & 30 deletions cpp/src/Ice/ConnectionRequestHandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,49 +13,22 @@ using namespace std;
using namespace IceInternal;

ConnectionRequestHandler::ConnectionRequestHandler(const ReferencePtr& reference,
const Ice::ConnectionIPtr& connection,
bool compress) :
const Ice::ConnectionIPtr& connection,
bool compress) :
RequestHandler(reference),
_connection(connection),
_compress(compress)
{
}

RequestHandlerPtr
ConnectionRequestHandler::update(const RequestHandlerPtr& previousHandler, const RequestHandlerPtr& newHandler)
{
assert(previousHandler);
try
{
if(previousHandler.get() == this)
{
return newHandler;
}
else if(previousHandler->getConnection() == _connection)
{
//
// If both request handlers point to the same connection, we also
// update the request handler. See bug ICE-5489 for reasons why
// this can be useful.
//
return newHandler;
}
}
catch(const Ice::Exception&)
{
// Ignore.
}
return shared_from_this();
}

AsyncStatus
ConnectionRequestHandler::sendAsyncRequest(const ProxyOutgoingAsyncBasePtr& out)
{
return out->invokeRemote(_connection, _compress, _response);
}

void
ConnectionRequestHandler::asyncRequestCanceled(const OutgoingAsyncBasePtr& outAsync, exception_ptr ex)
ConnectionRequestHandler::asyncRequestCanceled(const OutgoingAsyncBasePtr& outAsync, std::exception_ptr ex)
{
_connection->asyncRequestCanceled(outAsync, ex);
}
Expand Down
2 changes: 0 additions & 2 deletions cpp/src/Ice/ConnectionRequestHandler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@ class ConnectionRequestHandler final :

ConnectionRequestHandler(const ReferencePtr&, const Ice::ConnectionIPtr&, bool);

virtual RequestHandlerPtr update(const RequestHandlerPtr&, const RequestHandlerPtr&);

virtual AsyncStatus sendAsyncRequest(const ProxyOutgoingAsyncBasePtr&);

virtual void asyncRequestCanceled(const OutgoingAsyncBasePtr&, std::exception_ptr);
Expand Down
4 changes: 2 additions & 2 deletions cpp/src/Ice/ObjectAdapterFactory.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ IceInternal::ObjectAdapterFactory::createObjectAdapter(const string& name, const
}

ObjectAdapterPtr
IceInternal::ObjectAdapterFactory::findObjectAdapter(const ObjectPrxPtr& proxy)
IceInternal::ObjectAdapterFactory::findObjectAdapter(const ReferencePtr& reference)
{
list<shared_ptr<ObjectAdapterI>> adapters;
{
Expand All @@ -193,7 +193,7 @@ IceInternal::ObjectAdapterFactory::findObjectAdapter(const ObjectPrxPtr& proxy)
{
try
{
if((*p)->isLocal(proxy))
if((*p)->isLocal(reference))
{
return *p;
}
Expand Down
2 changes: 1 addition & 1 deletion cpp/src/Ice/ObjectAdapterFactory.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ class ObjectAdapterFactory : public std::enable_shared_from_this<ObjectAdapterFa
void updateObservers(void (Ice::ObjectAdapterI::*)());

::Ice::ObjectAdapterPtr createObjectAdapter(const std::string&, const Ice::RouterPrxPtr&);
::Ice::ObjectAdapterPtr findObjectAdapter(const ::Ice::ObjectPrxPtr&);
::Ice::ObjectAdapterPtr findObjectAdapter(const ::IceInternal::ReferencePtr&);
void removeObjectAdapter(const ::Ice::ObjectAdapterPtr&);
void flushAsyncBatchRequests(const CommunicatorFlushBatchAsyncPtr&, ::Ice::CompressBatch) const;

Expand Down
3 changes: 1 addition & 2 deletions cpp/src/Ice/ObjectAdapterI.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -696,14 +696,13 @@ Ice::ObjectAdapterI::getDispatchQueue() const
#endif

bool
Ice::ObjectAdapterI::isLocal(const ObjectPrxPtr& proxy) const
Ice::ObjectAdapterI::isLocal(const ReferencePtr& ref) const
{
//
// NOTE: it's important that isLocal() doesn't perform any blocking operations as
// it can be called for AMI invocations if the proxy has no delegate set yet.
//

ReferencePtr ref = proxy->_getReference();
if(ref->isWellKnown())
{
//
Expand Down
2 changes: 1 addition & 1 deletion cpp/src/Ice/ObjectAdapterI.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ class ObjectAdapterI final : public ObjectAdapter, public std::enable_shared_fro
dispatch_queue_t getDispatchQueue() const final;
#endif

bool isLocal(const ObjectPrxPtr&) const;
bool isLocal(const IceInternal::ReferencePtr&) const;

void flushAsyncBatchRequests(const IceInternal::CommunicatorFlushBatchAsyncPtr&, CompressBatch);

Expand Down
4 changes: 2 additions & 2 deletions cpp/src/Ice/OutgoingAsync.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,7 +426,7 @@ ProxyOutgoingAsyncBase::retryException()
// require could end up waiting for the flush of the
// connection to be done.
//
_proxy->_updateRequestHandler(_handler, 0); // Clear request handler and always retry.
_proxy->_clearRequestHandler(_handler); // Clear request handler and always retry.
_instance->retryQueue()->add(shared_from_this(), 0);
}
catch(const Ice::Exception&)
Expand Down Expand Up @@ -535,7 +535,7 @@ ProxyOutgoingAsyncBase::invokeImpl(bool userThread)
}
catch(const RetryException&)
{
_proxy->_updateRequestHandler(_handler, 0); // Clear request handler and always retry.
_proxy->_clearRequestHandler(_handler); // Clear request handler and always retry.
}
catch(const Exception& ex)
{
Expand Down
20 changes: 6 additions & 14 deletions cpp/src/Ice/Proxy.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,7 +448,7 @@ Ice::ObjectPrx::_handleException(std::exception_ptr ex,
bool sent,
int& cnt)
{
_updateRequestHandler(handler, 0); // Clear the request handler
_clearRequestHandler(handler); // Clear the request handler

//
// We only retry local exception, system exceptions aren't retried.
Expand Down Expand Up @@ -509,7 +509,7 @@ Ice::ObjectPrx::_getRequestHandler()
return _requestHandler;
}
}
return _reference->getRequestHandler(shared_from_this());
return _setRequestHandler(_reference->getRequestHandler());
}

::IceInternal::RequestHandlerPtr
Expand All @@ -528,22 +528,14 @@ Ice::ObjectPrx::_setRequestHandler(const ::IceInternal::RequestHandlerPtr& handl
}

void
Ice::ObjectPrx::_updateRequestHandler(const ::IceInternal::RequestHandlerPtr& previous,
const ::IceInternal::RequestHandlerPtr& handler)
Ice::ObjectPrx::_clearRequestHandler(const ::IceInternal::RequestHandlerPtr& previous)
{
if(_reference->getCacheConnection() && previous)
if(_reference->getCacheConnection())
{
lock_guard lock(_mutex);
if(_requestHandler && _requestHandler.get() != handler.get())
if(_requestHandler && _requestHandler.get() == previous.get())
{
//
// Update the request handler only if "previous" is the same
// as the current request handler. This is called after
// connection binding by the connect request handler. We only
// replace the request handler if the current handler is the
// connect request handler.
//
_requestHandler = _requestHandler->update(previous, handler);
_requestHandler = nullptr;
}
}
}
Expand Down
8 changes: 4 additions & 4 deletions cpp/src/Ice/Reference.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -679,7 +679,7 @@ IceInternal::FixedReference::toProperty(const string&) const
}

RequestHandlerPtr
IceInternal::FixedReference::getRequestHandler(const Ice::ObjectPrxPtr& proxy) const
IceInternal::FixedReference::getRequestHandler() const
{
switch(getMode())
{
Expand Down Expand Up @@ -737,7 +737,7 @@ IceInternal::FixedReference::getRequestHandler(const Ice::ObjectPrxPtr& proxy) c
}

ReferencePtr ref = const_cast<FixedReference*>(this)->shared_from_this();
return proxy->_setRequestHandler(make_shared<ConnectionRequestHandler>(ref, _fixedConnection, compress));
return make_shared<ConnectionRequestHandler>(ref, _fixedConnection, compress);
}

BatchRequestQueuePtr
Expand Down Expand Up @@ -1399,10 +1399,10 @@ IceInternal::RoutableReference::clone() const
}

RequestHandlerPtr
IceInternal::RoutableReference::getRequestHandler(const Ice::ObjectPrxPtr& proxy) const
IceInternal::RoutableReference::getRequestHandler() const
{
return getInstance()->requestHandlerFactory()->getRequestHandler(
dynamic_pointer_cast<RoutableReference>(const_cast<RoutableReference*>(this)->shared_from_this()), proxy);
dynamic_pointer_cast<RoutableReference>(const_cast<RoutableReference*>(this)->shared_from_this()));
}

BatchRequestQueuePtr
Expand Down
6 changes: 3 additions & 3 deletions cpp/src/Ice/Reference.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ class Reference : public std::enable_shared_from_this<Reference>
//
// Get a suitable connection for this reference.
//
virtual RequestHandlerPtr getRequestHandler(const Ice::ObjectPrxPtr&) const = 0;
virtual RequestHandlerPtr getRequestHandler() const = 0;
virtual BatchRequestQueuePtr getBatchRequestQueue() const = 0;

virtual bool operator==(const Reference&) const;
Expand Down Expand Up @@ -222,7 +222,7 @@ class FixedReference : public Reference
virtual void streamWrite(Ice::OutputStream*) const;
virtual Ice::PropertyDict toProperty(const std::string&) const;

virtual RequestHandlerPtr getRequestHandler(const Ice::ObjectPrxPtr&) const;
virtual RequestHandlerPtr getRequestHandler() const;
virtual BatchRequestQueuePtr getBatchRequestQueue() const;

virtual bool operator==(const Reference&) const;
Expand Down Expand Up @@ -288,7 +288,7 @@ class RoutableReference : public Reference

virtual ReferencePtr clone() const;

virtual RequestHandlerPtr getRequestHandler(const Ice::ObjectPrxPtr&) const;
virtual RequestHandlerPtr getRequestHandler() const;
virtual BatchRequestQueuePtr getBatchRequestQueue() const;

void getConnection(const GetConnectionCallbackPtr&) const;
Expand Down
2 changes: 0 additions & 2 deletions cpp/src/Ice/RequestHandler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,6 @@ class RequestHandler : public CancellationHandler

RequestHandler(const ReferencePtr&);

virtual RequestHandlerPtr update(const RequestHandlerPtr&, const RequestHandlerPtr&) = 0;

virtual AsyncStatus sendAsyncRequest(const ProxyOutgoingAsyncBasePtr&) = 0;

virtual Ice::ConnectionIPtr getConnection() = 0;
Expand Down
Loading
Loading