diff options
Diffstat (limited to 'cpp')
| -rw-r--r-- | cpp/demo/Ice/session/Session.ice | 11 | ||||
| -rwxr-xr-x | cpp/demo/Ice/session/SessionFactoryI.cpp | 1 | ||||
| -rwxr-xr-x | cpp/demo/Ice/session/SessionFactoryI.h | 17 | ||||
| -rwxr-xr-x | cpp/demo/Ice/session/SessionI.cpp | 14 | ||||
| -rwxr-xr-x | cpp/demo/Ice/session/SessionI.h | 13 |
5 files changed, 51 insertions, 5 deletions
diff --git a/cpp/demo/Ice/session/Session.ice b/cpp/demo/Ice/session/Session.ice index 32fbdd43a53..81febb929c1 100644 --- a/cpp/demo/Ice/session/Session.ice +++ b/cpp/demo/Ice/session/Session.ice @@ -10,6 +10,13 @@ #ifndef SESSION_ICE #define SESSION_ICE +// XXX Regarding the /** comments: Either do it correctly, or get rid +// of them (use normal comments, use //). If you want to use /** +// properly, then add parameter and return value descriptions. I do +// not think that this is necessary, so I recommend to only use normal +// comments. Then you can also get rid of trivial comments, like +// "Hello object". + module Demo { @@ -39,6 +46,10 @@ interface Session **/ idempotent void refresh(); + // XXX "Mark" as destroyed? This should read "Destroy the + // session". Whether or not it's marked internally is irrelevant + // for the description of the interface. + /** Mark the session as destroyed. */ void destroy(); }; diff --git a/cpp/demo/Ice/session/SessionFactoryI.cpp b/cpp/demo/Ice/session/SessionFactoryI.cpp index ed111333b93..13bd2f94c5d 100755 --- a/cpp/demo/Ice/session/SessionFactoryI.cpp +++ b/cpp/demo/Ice/session/SessionFactoryI.cpp @@ -117,6 +117,7 @@ SessionFactoryI::destroy() { Lock sync(*this); + // XXX When _reapThread is out of this class, simply place this code after the waitForShutdown(). _reapThread->terminate(); _reapThread->getThreadControl().join(); _reapThread = 0; diff --git a/cpp/demo/Ice/session/SessionFactoryI.h b/cpp/demo/Ice/session/SessionFactoryI.h index 8e897e2fe5a..5ace415efd9 100755 --- a/cpp/demo/Ice/session/SessionFactoryI.h +++ b/cpp/demo/Ice/session/SessionFactoryI.h @@ -26,6 +26,8 @@ public: virtual ~ReapThread(); virtual void run(); + + // XXX Rename to destroy(). void terminate(); private: @@ -51,15 +53,26 @@ public: private: - const Ice::ObjectAdapterPtr _adapter; - const IceUtil::Time _timeout; + const Ice::ObjectAdapterPtr _adapter; // XXX Get rid of this, not needed. + const IceUtil::Time _timeout; // XXX Get rid of this, see other comments. + + // XXX Why does the factory have to know the reaper thread? The + // sessions should know, they can register themselves directly + // with the reaper thread. + ReapThreadPtr _reapThread; + + // XXX Get rid of this. Use a map<Ice::Identity, SessionIPtr>. struct SessionId { SessionId(const SessionIPtr& s, const ::Ice::Identity& i) : session(s), id(i) { } const SessionIPtr session; const ::Ice::Identity id; }; + + // XXX Why does SessionFactoryI keep a list of sessions? Clearly, + // only the reaper must know it, which currently simply delegates + // work to the SessionFactoryI, instead of doing it directly. std::list<SessionId> _sessions; }; diff --git a/cpp/demo/Ice/session/SessionI.cpp b/cpp/demo/Ice/session/SessionI.cpp index ec4ffc296ef..75569accf48 100755 --- a/cpp/demo/Ice/session/SessionI.cpp +++ b/cpp/demo/Ice/session/SessionI.cpp @@ -13,6 +13,8 @@ using namespace std; using namespace Demo; +// XXX Add using namespace Ice. + class HelloI : public Hello { public: @@ -24,6 +26,8 @@ public: ~HelloI() { + // XXX Use real output, something like "`Hello' object #xxx + // destroyed.", not programming language style output. cout << _id << ": ~Hello" << endl; } @@ -49,6 +53,7 @@ SessionI::SessionI(const Ice::ObjectAdapterPtr& adapter, const IceUtil::Time& ti SessionI::~SessionI() { + // XXX Remove, or add user-readable comment. cout << "~SessionI" << endl; } @@ -56,6 +61,7 @@ HelloPrx SessionI::createHello(const Ice::Current&) { Lock sync(*this); + // XXX Check for destruction w/ ObjectNotExistException? HelloPrx hello = HelloPrx::uncheckedCast(_adapter->addWithUUID(new HelloI(_nextId++))); _objs.push_back(hello); return hello; @@ -65,6 +71,7 @@ void SessionI::refresh(const Ice::Current& c) { Lock sync(*this); + // XXX Check for destruction w/ ObjectNotExistException? _refreshTime = IceUtil::Time::now(); } @@ -72,20 +79,27 @@ void SessionI::destroy(const Ice::Current& c) { Lock sync(*this); + // XXX Check for destruction w/ ObjectNotExistException? _destroy = true; + // XXX Add cleanup from the so-called "destroyCallback" here. } bool SessionI::destroyed() const { + // XXX This should only check for _destroy. A reaper thread should + // call destroy() if there was a timeout. Lock sync(*this); return _destroy || (IceUtil::Time::now() - _refreshTime) > _timeout; } +// XXX Get rid of this function, remove the hello objects from the +// object adapter in destroy(). void SessionI::destroyCallback() { Lock sync(*this); + // XXX Real output please, that is appropriate for a demo. cout << "SessionI::destroyCallback: _destroy=" << _destroy << " timeout=" << ((IceUtil::Time::now()-_refreshTime) > _timeout) << endl; for(list<HelloPrx>::const_iterator p = _objs.begin(); p != _objs.end(); ++p) diff --git a/cpp/demo/Ice/session/SessionI.h b/cpp/demo/Ice/session/SessionI.h index 9dd23990d01..2d46ef27d3d 100755 --- a/cpp/demo/Ice/session/SessionI.h +++ b/cpp/demo/Ice/session/SessionI.h @@ -28,11 +28,11 @@ public: virtual void refresh(const Ice::Current&); virtual void destroy(const Ice::Current&); - // XXX Should be private, with SessionFactoryI being a friend. + // XXX Should be private, with SessionFactoryI being a friend. (Or Reaper being a friend, see the other comments.) // Return true if the session is destroyed, false otherwise. bool destroyed() const; - // XXX Should be private, with SessionFactoryI being a friend. + // XXX Should be private, with SessionFactoryI being a friend. (Or Reaper being a friend, see the other comments.) // XXX The name is wrong. It's not a callback, it's a call. // XXX Why have this function at all? Why not do whatever this function does directly in destroy()? // per-client allocated resources. @@ -40,9 +40,16 @@ public: private: - const Ice::ObjectAdapterPtr _adapter; + const Ice::ObjectAdapterPtr _adapter; // XXX Get rid of this (you can after all the other XXX's have been fixed). + + // XXX Get rid of this. Only the reaper has to know this. It can + // call a timestamp() function, compare the elapsed time with its + // _timeout value, and call destroy() and reap if timed out. + const IceUtil::Time _timeout; // How long until the session times out. bool _destroy; // true if destroy() was called, false otherwise. + + // XXX Rename to _timestamp; IceUtil::Time _refreshTime; // The last time the session was refreshed. int _nextId; // The id of the next hello object. This is used for tracing purposes. |