addressed comments added by Marc.

8 files changed, 94 insertions, 256 deletions

diff --git a/cpp/demo/Ice/session/Makefile b/cpp/demo/Ice/session/Makefile
index 691cb600ab8..fd8c5be428d 100755
--- a/cpp/demo/Ice/session/Makefile
+++ b/cpp/demo/Ice/session/Makefile
@@ -18,7 +18,8 @@ OBJS		= Session.o
 
 COBJS		= Client.o
 
-SOBJS		= SessionFactoryI.o \
+SOBJS		= ReapThread.o \
+		  SessionFactoryI.o \
 		  SessionI.o \
 		  Server.o
 
diff --git a/cpp/demo/Ice/session/Server.cpp b/cpp/demo/Ice/session/Server.cpp
index 48fbd2c3dc7..d9551e7ddfc 100644
--- a/cpp/demo/Ice/session/Server.cpp
+++ b/cpp/demo/Ice/session/Server.cpp
@@ -9,6 +9,7 @@
 
 #include <Ice/Ice.h>
 #include <SessionFactoryI.h>
+#include <ReapThread.h>
 
 using namespace std;
 using namespace Demo;
@@ -17,11 +18,15 @@ int
 run(int argc, char* argv[], const Ice::CommunicatorPtr& communicator)
 {
     Ice::ObjectAdapterPtr adapter = communicator->createObjectAdapter("SessionFactory");
-    SessionFactoryIPtr factory = new SessionFactoryI(adapter);
-    adapter->add(factory, Ice::stringToIdentity("SessionFactory"));
+    ReapThreadPtr reaper = new ReapThread(IceUtil::Time::seconds(10));
+    adapter->add(new SessionFactoryI(reaper), Ice::stringToIdentity("SessionFactory"));
     adapter->activate();
+    reaper->start();
     communicator->waitForShutdown();
-    factory->destroy();
+
+    reaper->terminate();
+    reaper->getThreadControl().join();
+
     return EXIT_SUCCESS;
 }
 
diff --git a/cpp/demo/Ice/session/Session.ice b/cpp/demo/Ice/session/Session.ice
index 81febb929c1..564960b9a23 100644
--- a/cpp/demo/Ice/session/Session.ice
+++ b/cpp/demo/Ice/session/Session.ice
@@ -10,57 +10,44 @@
 #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
 {
 
-/** Hello object. */
 interface Hello
 {
-    /** Send a greeting. */
+    // Send a greeting.
     nonmutating void sayHello();
 };
 
-/**
- * The session object. This is used to create per-session objects on
- * behalf of the client. If it is not refreshed on a periodic basis it
- * will be automatically reclaimed by the session factory.
- */
+//
+// The session object. This is used to create per-session objects on
+// behalf of the client. If it is not refreshed on a periodic basis it
+// will be automatically reclaimed by the session factory.
+///
 interface Session
 {
-    /**
-     * Create a new per-session hello object. The created object will
-     * be automatically destroyed when the session is destroyed.
-     */
+    //
+    // Create a new per-session hello object. The created object will
+    // be automatically destroyed when the session is destroyed.
+    //
     Hello* createHello();
 
-    /**
-     * Refresh a session. If a session is not refreshed on a regular
-     * basis by the client it will be automatically destroyed.
-     **/
+    //
+    // Refresh a session. If a session is not refreshed on a regular
+    // basis by the client it will be automatically destroyed.
+    //
     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. */
+    //
+    // Destroy the session.
+    //
     void destroy();
 };
 
-/** The SessionFactory. */
 interface SessionFactory
 {
-    /** Create a new session. */
     Session* create();
 
-    /** Shutdown the server. */
     idempotent void shutdown();
 };
 
diff --git a/cpp/demo/Ice/session/SessionFactoryI.cpp b/cpp/demo/Ice/session/SessionFactoryI.cpp
index 13bd2f94c5d..72fe47e5af1 100755
--- a/cpp/demo/Ice/session/SessionFactoryI.cpp
+++ b/cpp/demo/Ice/session/SessionFactoryI.cpp
@@ -13,55 +13,13 @@
 using namespace std;
 using namespace Demo;
 
-// XXX Why does the reaper thread have to know the factory?
-ReapThread::ReapThread(const SessionFactoryIPtr& factory, const IceUtil::Time& timeout) :
-    _terminated(false),
-    _timeout(timeout),
-    _factory(factory)
+SessionFactoryI::SessionFactoryI(const ReapThreadPtr& reapThread) :
+    _reapThread(reapThread)
 {
 }
 
-ReapThread::~ReapThread()
-{
-    cout << "~ReapThread" << endl;
-}
-
-void
-ReapThread::run()
-{
-    Lock sync(*this);
-    while(!_terminated)
-    {
-	timedWait(_timeout);
-	if(!_terminated)
-	{
-	    assert(_factory);
-	    _factory->reap();
-	}
-    }
-}
-
-void
-ReapThread::terminate()
-{
-    Lock sync(*this);
-    _terminated = true;
-    notify();
-    // Drop the cyclic reference count.
-    _factory = 0;
-}
-
-SessionFactoryI::SessionFactoryI(const Ice::ObjectAdapterPtr& adapter) :
-    _adapter(adapter),
-    _timeout(IceUtil::Time::seconds(10)),
-    _reapThread(new ReapThread(this, _timeout))
-{
-    _reapThread->start();
-}
-
 SessionFactoryI::~SessionFactoryI()
 {
-    cout << "~SessionFactoryI" << endl;
 }
 
 SessionPrx
@@ -69,9 +27,9 @@ SessionFactoryI::create(const Ice::Current& c)
 {
     Lock sync(*this);
 
-    SessionIPtr session = new SessionI(_adapter, _timeout);
-    SessionPrx proxy = SessionPrx::uncheckedCast(_adapter->addWithUUID(session));
-    _sessions.push_back(SessionId(session, proxy->ice_getIdentity()));
+    SessionIPtr session = new SessionI;
+    SessionPrx proxy = SessionPrx::uncheckedCast(c.adapter->addWithUUID(session));
+    _reapThread->add(proxy, session);
     return proxy;
 }
 
@@ -81,54 +39,3 @@ SessionFactoryI::shutdown(const ::Ice::Current& c)
     cout << "Shutting down..." << endl;
     c.adapter->getCommunicator()->shutdown();
 }
-
-void
-SessionFactoryI::reap()
-{
-    Lock sync(*this);
-
-    list<SessionId>::iterator p = _sessions.begin();
-    while(p != _sessions.end())
-    {
-	if(p->session->destroyed())
-	{
-	    p->session->destroyCallback();
-	    try
-	    {
-		_adapter->remove(p->id);
-	    }
-	    catch(const Ice::ObjectAdapterDeactivatedException&)
-	    {
-		// This method can be called while the server is
-		// shutting down, in which case this exception is
-		// expected.
-	    }
-    	    p = _sessions.erase(p);
-	}
-	else
-	{
-	    ++p;
-	}
-    }
-}
-
-void
-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;
-
-    for(list<SessionId>::const_iterator p = _sessions.begin(); p != _sessions.end(); ++p)
-    {
-	p->session->destroyCallback();
-
-    	// When the session factory is destroyed the OA is deactivated
-    	// and all servants have been removed so calling remove on the
-    	// OA is not necessary.
-    }
-    _sessions.clear();
-}
diff --git a/cpp/demo/Ice/session/SessionFactoryI.h b/cpp/demo/Ice/session/SessionFactoryI.h
index 5ace415efd9..4be5f984b0c 100755
--- a/cpp/demo/Ice/session/SessionFactoryI.h
+++ b/cpp/demo/Ice/session/SessionFactoryI.h
@@ -10,70 +10,32 @@
 #ifndef SESSION_FACTORY_I_H
 #define SESSION_FACTORY_I_H
 
-#include <IceUtil/Thread.h>
-#include <SessionI.h>
-
-#include <list>
-
-class SessionFactoryI;
-typedef IceUtil::Handle<SessionFactoryI> SessionFactoryIPtr;
-
-class ReapThread : public IceUtil::Thread, public IceUtil::Monitor<IceUtil::Mutex>
-{
-public:
-
-    ReapThread(const SessionFactoryIPtr&, const IceUtil::Time&);
-    virtual ~ReapThread();
-
-    virtual void run();
-
-    // XXX Rename to destroy().
-    void terminate();
-
-private:
-
-    bool _terminated;
-    const IceUtil::Time _timeout;
-    SessionFactoryIPtr _factory;
-};
-typedef IceUtil::Handle<ReapThread> ReapThreadPtr;
+#include <Session.h>
+#include <ReapThread.h>
 
 class SessionFactoryI : public ::Demo::SessionFactory, public IceUtil::Mutex
 {
 public:
 
-    SessionFactoryI(const Ice::ObjectAdapterPtr&);
+    SessionFactoryI(const ReapThreadPtr&);
     virtual ~SessionFactoryI();
 
     virtual ::Demo::SessionPrx create(const ::Ice::Current&);
     virtual void shutdown(const Ice::Current&);
 
-    void reap();
-    void destroy();
-
 private:
 
-    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;
+    //
+    // If I do this it means that either the reap thread must be a
+    // singleton, or I need to pass the reap thread to session and
+    // tell the session its proxy which it currently does not
+    // know. Since the session factory knows both, this seems like a
+    // better solution.
+    //
+    const ReapThreadPtr _reapThread;
 };
 
 #endif
diff --git a/cpp/demo/Ice/session/SessionI.cpp b/cpp/demo/Ice/session/SessionI.cpp
index 75569accf48..add61597ff2 100755
--- a/cpp/demo/Ice/session/SessionI.cpp
+++ b/cpp/demo/Ice/session/SessionI.cpp
@@ -14,6 +14,7 @@ using namespace std;
 using namespace Demo;
 
 // XXX Add using namespace Ice.
+// Style: the other demos do not use namespace Ice.
 
 class HelloI : public Hello
 {
@@ -26,15 +27,13 @@ public:
 
     ~HelloI()
     {
-	// XXX Use real output, something like "`Hello' object #xxx
-	// destroyed.", not programming language style output.
-	cout << _id << ": ~Hello" << endl;
+	cout << "Hello object #" << _id << " destroyed" << endl;
     }
 
     void
     sayHello(const Ice::Current&) const
     {
-	cout << _id << ": Hello World!" << endl;
+	cout << "Hello object #" << _id << " says 'Hello World!'" << endl;
     }
 
 private:
@@ -42,27 +41,11 @@ private:
     const int _id;
 };
 
-SessionI::SessionI(const Ice::ObjectAdapterPtr& adapter, const IceUtil::Time& timeout) :
-    _adapter(adapter),
-    _timeout(timeout),
-    _destroy(false),
-    _refreshTime(IceUtil::Time::now()),
-    _nextId(0)
-{
-}
-
-SessionI::~SessionI()
-{
-    // XXX Remove, or add user-readable comment.
-    cout << "~SessionI" << endl;
-}
-
 HelloPrx
-SessionI::createHello(const Ice::Current&)
+SessionI::createHello(const Ice::Current& c)
 {
     Lock sync(*this);
-    // XXX Check for destruction w/ ObjectNotExistException?
-    HelloPrx hello = HelloPrx::uncheckedCast(_adapter->addWithUUID(new HelloI(_nextId++)));
+    HelloPrx hello = HelloPrx::uncheckedCast(c.adapter->addWithUUID(new HelloI(_nextId++)));
     _objs.push_back(hello);
     return hello;
 }
@@ -71,48 +54,47 @@ void
 SessionI::refresh(const Ice::Current& c)
 {
     Lock sync(*this);
-    // XXX Check for destruction w/ ObjectNotExistException?
-    _refreshTime = IceUtil::Time::now();
+    _timestamp = IceUtil::Time::now();
 }
 
 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;
+    cout << "The session #" << Ice::identityToString(c.id) << " is now destroyed." << endl;
+    try
+    {
+	c.adapter->remove(c.id);
+	for(list<HelloPrx>::const_iterator p = _objs.begin(); p != _objs.end(); ++p)
+	{
+	    c.adapter->remove((*p)->ice_getIdentity());
+	}
+    }
+    catch(const Ice::ObjectAdapterDeactivatedException&)
+    {
+	// This method is called on shutdown of the server, in which
+	// case this exception is expected.
+    }
+
+    _objs.clear();
 }
 
-// XXX Get rid of this function, remove the hello objects from the
-// object adapter in destroy().
-void
-SessionI::destroyCallback()
+IceUtil::Time
+SessionI::timestamp() const
 {
     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)
+    if(_destroy)
     {
-	try
-	{
-	    _adapter->remove((*p)->ice_getIdentity());
-	}
-	catch(const Ice::ObjectAdapterDeactivatedException&)
-	{
-	    // This method is called on shutdown of the server, in
-	    // which case this exception is expected.
-	}
+	throw Ice::ObjectNotExistException(__FILE__, __LINE__);
     }
-    _objs.clear();
+    return _timestamp;
+}
+
+SessionI::SessionI() :
+    _timestamp(IceUtil::Time::now()),
+    _nextId(0),
+    _destroy(false)
+{
 }
diff --git a/cpp/demo/Ice/session/SessionI.h b/cpp/demo/Ice/session/SessionI.h
index 2d46ef27d3d..512a7acc4a1 100755
--- a/cpp/demo/Ice/session/SessionI.h
+++ b/cpp/demo/Ice/session/SessionI.h
@@ -15,45 +15,31 @@
 
 // XXX Get rid of leading ::, i.e., use Demo::, not ::Demo::
 // (everywhere).
+// Style: The other demos all use ::Demo::
 
 class SessionI : public ::Demo::Session, public IceUtil::Mutex
 {
 public:
 
-    // XXX Should be private, with SessionFactoryI being a friend.
-    SessionI(const Ice::ObjectAdapterPtr&, const IceUtil::Time&);
-    virtual ~SessionI();
-
     virtual ::Demo::HelloPrx createHello(const Ice::Current&);
     virtual void refresh(const Ice::Current&);
     virtual void destroy(const Ice::Current&);
 
-    // 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. (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.
-    void destroyCallback();
-
 private:
 
-    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.
+    // Only the ReapThread is interested in the timestamp.
+    friend class ReapThread;
+    IceUtil::Time timestamp() const;
 
-    const IceUtil::Time _timeout; // How long until the session times out.
-    bool _destroy; // true if destroy() was called, false otherwise.
+    // Only the session factory can create sessions.
+    friend class SessionFactoryI;
+    SessionI();
 
-    // XXX Rename to _timestamp;
-    IceUtil::Time _refreshTime; // The last time the session was refreshed.
+    IceUtil::Time _timestamp; // The last time the session was refreshed.
     
     int _nextId; // The id of the next hello object. This is used for tracing purposes.
     std::list< ::Demo::HelloPrx> _objs; // List of per-client allocated Hello objects.
+    bool _destroy;
 };
 typedef IceUtil::Handle<SessionI> SessionIPtr;
 
diff --git a/cpp/demo/Ice/session/sessionS.dsp b/cpp/demo/Ice/session/sessionS.dsp
index ff110834154..f7c5f0f2613 100644
--- a/cpp/demo/Ice/session/sessionS.dsp
+++ b/cpp/demo/Ice/session/sessionS.dsp
@@ -91,6 +91,10 @@ LINK32=link.exe
 # PROP Default_Filter "cpp;c;cxx;rc;def;r;odl;idl;hpj;bat"

 # Begin Source File

 

+SOURCE=.\ReapThread.cpp

+# End Source File

+# Begin Source File

+

 SOURCE=.\Server.cpp

 # End Source File

 # Begin Source File

@@ -111,6 +115,10 @@ SOURCE=.\SessionI.cpp
 # PROP Default_Filter "h;hpp;hxx;hm;inl"

 # Begin Source File

 

+SOURCE=.\ReapThread.h

+# End Source File

+# Begin Source File

+

 SOURCE=.\Session.h

 # End Source File

 # Begin Source File