more changes to the session demo.

9 files changed, 130 insertions, 132 deletions

diff --git a/cpp/demo/Ice/session/Client.cpp b/cpp/demo/Ice/session/Client.cpp
index 84bc1d15e97..62050a085a4 100755
--- a/cpp/demo/Ice/session/Client.cpp
+++ b/cpp/demo/Ice/session/Client.cpp
@@ -67,23 +67,42 @@ private:
 
 typedef IceUtil::Handle<SessionRefreshThread> SessionRefreshThreadPtr;
 
-void
-menu()
+class SessionClient : public Ice::Application
 {
-    cout <<
-	"usage:\n"
-	"c:     create a new per-client hello object\n"
-	"0-9:   send a greeting to a hello object\n"
-	"s:     shutdown the server\n"
-	"x:     exit\n"
-	"t:     exit without destroying the session\n"
-	"?:     help\n";
+public:
+
+    virtual int run(int, char*[]);
+
+private:
+
+    void menu();
+    string trim(const string&);
+};
+
+int
+main(int argc, char* argv[])
+{
+    SessionClient app;
+    return app.main(argc, argv, "config");
 }
 
 int
-run(int argc, char* argv[], const Ice::CommunicatorPtr& communicator)
+SessionClient::run(int argc, char* argv[])
 {
-    Ice::PropertiesPtr properties = communicator->getProperties();
+    string name;
+    do
+    {
+	cout << "Please enter your name ==> ";
+	cin >> name;
+	if(!cin.good())
+	{
+	    return EXIT_FAILURE;
+	}
+	name = trim(name);
+    }
+    while(name.size() == 0);
+
+    Ice::PropertiesPtr properties = communicator()->getProperties();
     const char* proxyProperty = "SessionFactory.Proxy";
     string proxy = properties->getProperty(proxyProperty);
     if(proxy.empty())
@@ -92,7 +111,7 @@ run(int argc, char* argv[], const Ice::CommunicatorPtr& communicator)
 	return EXIT_FAILURE;
     }
 
-    Ice::ObjectPrx base = communicator->stringToProxy(proxy);
+    Ice::ObjectPrx base = communicator()->stringToProxy(proxy);
     SessionFactoryPrx factory = SessionFactoryPrx::checkedCast(base);
     if(!factory)
     {
@@ -100,10 +119,10 @@ run(int argc, char* argv[], const Ice::CommunicatorPtr& communicator)
 	return EXIT_FAILURE;
     }
 
-    SessionPrx session = factory->create();
+    SessionPrx session = factory->create(name);
 
     SessionRefreshThreadPtr refresh = new SessionRefreshThread(
-	communicator->getLogger(), IceUtil::Time::seconds(5), session);
+	communicator()->getLogger(), IceUtil::Time::seconds(5), session);
     refresh->start();
 
     vector<HelloPrx> hellos;
@@ -140,7 +159,7 @@ run(int argc, char* argv[], const Ice::CommunicatorPtr& communicator)
 	    else if(c == 'c')
 	    {
 		hellos.push_back(session->createHello());
-		cout << "Created hello object #" << hellos.size() - 1 << endl;
+		cout << "Created hello object " << hellos.size() - 1 << endl;
 	    }
 	    else if(c == 's')
 	    {
@@ -182,39 +201,27 @@ run(int argc, char* argv[], const Ice::CommunicatorPtr& communicator)
     return EXIT_SUCCESS;
 }
 
-// XXX Rewrite to use Application. See ../callback for an example.
-
-int
-main(int argc, char* argv[])
+void
+SessionClient::menu()
 {
-    int status;
-    Ice::CommunicatorPtr communicator;
-
-    try
-    {
-	Ice::PropertiesPtr properties = Ice::createProperties();
-        properties->load("config");
-	communicator = Ice::initializeWithProperties(argc, argv, properties);
-	status = run(argc, argv, communicator);
-    }
-    catch(const Ice::Exception& ex)
-    {
-	cerr << ex << endl;
-	status = EXIT_FAILURE;
-    }
+    cout <<
+	"usage:\n"
+	"c:     create a new per-client hello object\n"
+	"0-9:   send a greeting to a hello object\n"
+	"s:     shutdown the server\n"
+	"x:     exit\n"
+	"t:     exit without destroying the session\n"
+	"?:     help\n";
+}
 
-    if(communicator)
+string
+SessionClient::trim(const string& s)
+{
+    static const string delims = "\t\r\n ";
+    string::size_type last = s.find_last_not_of(delims);
+    if(last != string::npos)
     {
-	try
-	{
-	    communicator->destroy();
-	}
-	catch(const Ice::Exception& ex)
-	{
-	    cerr << ex << endl;
-	    status = EXIT_FAILURE;
-	}
+	return s.substr(s.find_first_not_of(delims), last+1);
     }
-
-    return status;
+    return s;
 }
diff --git a/cpp/demo/Ice/session/ReapThread.cpp b/cpp/demo/Ice/session/ReapThread.cpp
index 9380aee84fa..02f66ec7284 100755
--- a/cpp/demo/Ice/session/ReapThread.cpp
+++ b/cpp/demo/Ice/session/ReapThread.cpp
@@ -29,10 +29,6 @@ ReapThread::instance()
     return _instance;
 }
 
-ReapThread::~ReapThread()
-{
-}
-
 void
 ReapThread::run()
 {
@@ -44,24 +40,21 @@ ReapThread::run()
 
 	if(!_terminated)
 	{
-	    // XXX Session destruction may take time in a real-world
-	    // example. Therefore now should be computed in the loop
-	    // for each iteration.
-	    IceUtil::Time now = IceUtil::Time::now();
-	    map<SessionPrx, SessionIPtr>::iterator p = _sessions.begin();
+	    list<SessionProxyPair>::iterator p = _sessions.begin();
 	    while(p != _sessions.end())
 	    {
 		try
 		{
-		    if((now - p->second->timestamp()) > _timeout)
+		    //
+		    // Session destruction may take time in a
+		    // real-world example. Therefore the current time
+		    // is computed for each iteration.
+		    //
+		    if((IceUtil::Time::now() - p->session->timestamp()) > _timeout)
 		    {
-			cout << "The session #" << Ice::identityToString(p->first->ice_getIdentity())
-			     << " has timed out." << endl;
-			p->first->destroy();
-			// XXX This can be simplified to _sessions.erase(p++);
-			map<SessionPrx, SessionIPtr>::iterator tmp = p;
-			++p;
-			_sessions.erase(tmp);
+			cout << "The session " << p->proxy->getName() << " has timed out." << endl;
+			p->proxy->destroy();
+    	    	    	p = _sessions.erase(p);
 		    }
 		    else
 		    {
@@ -70,10 +63,7 @@ ReapThread::run()
     	    	}
 		catch(const Ice::ObjectNotExistException&)
 		{
-		    // XXX This can be simplified to _sessions.erase(p++);
-		    map<SessionPrx, SessionIPtr>::iterator tmp = p;
-		    ++p;
-		    _sessions.erase(tmp);
+		    p = _sessions.erase(p);
 		}
 	    }
 	}
@@ -88,11 +78,11 @@ ReapThread::terminate()
     _terminated = true;
     notify();
 
-    for(map<SessionPrx, SessionIPtr>::const_iterator p = _sessions.begin(); p != _sessions.end(); ++p)
+    for(list<SessionProxyPair>::const_iterator p = _sessions.begin(); p != _sessions.end(); ++p)
     {
 	try
 	{
-	    p->first->destroy();
+	    p->proxy->destroy();
 	}
 	catch(const Ice::Exception&)
 	{
@@ -107,10 +97,7 @@ void
 ReapThread::add(const SessionPrx& proxy, const SessionIPtr& session)
 {
     Lock sync(*this);
-    // XXX Don't use make_pair, it's not portable. We had to remove it
-    // from most of our code, because the Sun compiler and others had
-    // problems with it.y
-    _sessions.insert(make_pair(proxy, session));
+    _sessions.push_back(SessionProxyPair(proxy, session));
 }
 
 ReapThread::ReapThread() :
diff --git a/cpp/demo/Ice/session/ReapThread.h b/cpp/demo/Ice/session/ReapThread.h
index 012dac82f47..33fdc7333d9 100755
--- a/cpp/demo/Ice/session/ReapThread.h
+++ b/cpp/demo/Ice/session/ReapThread.h
@@ -14,16 +14,17 @@
 #include <IceUtil/StaticMutex.h>
 #include <SessionI.h>
 
+#include <list>
+
 class ReapThread;
 typedef IceUtil::Handle<ReapThread> ReapThreadPtr;
+
 class ReapThread : public IceUtil::Thread, public IceUtil::Monitor<IceUtil::Mutex>
 {
 public:
 
     static ReapThreadPtr& instance();
 
-    virtual ~ReapThread(); // XXX Destructor does nothing, get rid of it.
-
     virtual void run();
 
     void terminate();
@@ -36,10 +37,14 @@ private:
 
     const IceUtil::Time _timeout;
     bool _terminated;
-    // XXX Why is this a map and not simply a list of
-    // pair<Demo::SessionPrx, SessionIPtr> (or a list of structs with
-    // these elements)? The sorting of a map is needed nowhere.
-    std::map< Demo::SessionPrx, SessionIPtr> _sessions;
+    struct SessionProxyPair
+    {
+	SessionProxyPair(const Demo::SessionPrx& p, const SessionIPtr& s) :
+	    proxy(p), session(s) { }
+	const Demo::SessionPrx proxy;
+	const SessionIPtr session;
+    };
+    std::list<SessionProxyPair> _sessions;
 
     static ReapThreadPtr _instance;
     static IceUtil::StaticMutex _instanceMutex;
diff --git a/cpp/demo/Ice/session/Server.cpp b/cpp/demo/Ice/session/Server.cpp
index 6199c3d5025..ab2db04de6a 100644
--- a/cpp/demo/Ice/session/Server.cpp
+++ b/cpp/demo/Ice/session/Server.cpp
@@ -7,7 +7,6 @@
 //
 // **********************************************************************
 
-#include <Ice/Ice.h> // XXX Get rid of this, see other comments.
 #include <SessionFactoryI.h>
 #include <ReapThread.h>
 
diff --git a/cpp/demo/Ice/session/Session.ice b/cpp/demo/Ice/session/Session.ice
index 3b887b64f26..ee9284aed1a 100644
--- a/cpp/demo/Ice/session/Session.ice
+++ b/cpp/demo/Ice/session/Session.ice
@@ -20,15 +20,8 @@ interface Hello
 
 //
 // 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.
-//
-// XXX Comment is wrong. First, sessions are not reclaimed - they are
-// destroyed. Second, the factory doesn't do this. Third, it is
-// irrelevant for the interface description to explain what
-// programming language construct exactly destroys expired
-// sessions. So this should simply read: "If it is not refreshed on a
-// periodic basis, it will be automatically destroyed."
+// behalf of the client. If the session is not refreshed on a periodic
+// basis, it will be automatically destroyed.
 //
 interface Session
 {
@@ -44,6 +37,8 @@ interface Session
     //
     idempotent void refresh();
 
+    nonmutating string getName();
+
     //
     // Destroy the session explicitly.
     //
@@ -52,11 +47,7 @@ interface Session
 
 interface SessionFactory
 {
-    // XXX I think you should pass a user name parameter in here. The
-    // client could then first ask "What is your name"? Then the
-    // server could print messages like "Hello object #XXX says `Hello
-    // Foo!'"
-    Session* create();
+    Session* create(string name);
 
     idempotent void shutdown();
 };
diff --git a/cpp/demo/Ice/session/SessionFactoryI.cpp b/cpp/demo/Ice/session/SessionFactoryI.cpp
index 03fa652e862..e87dc33df75 100755
--- a/cpp/demo/Ice/session/SessionFactoryI.cpp
+++ b/cpp/demo/Ice/session/SessionFactoryI.cpp
@@ -7,27 +7,16 @@
 //
 // **********************************************************************
 
-#include <Ice/Ice.h> // XXX Get rid of this, SessionFactoryI.cpp must include this.
 #include <SessionFactoryI.h>
 #include <ReapThread.h>
 
 using namespace std;
 using namespace Demo;
 
-SessionFactoryI::SessionFactoryI()
-{
-}
-
-SessionFactoryI::~SessionFactoryI()
-{
-}
-
 SessionPrx
-SessionFactoryI::create(const Ice::Current& c)
+SessionFactoryI::create(const string& name, const Ice::Current& c)
 {
-    Lock sync(*this); // XXX What is the mutex lock needed for?
-
-    SessionIPtr session = new SessionI;
+    SessionIPtr session = new SessionI(name);
     SessionPrx proxy = SessionPrx::uncheckedCast(c.adapter->addWithUUID(session));
     ReapThread::instance()->add(proxy, session);
     return proxy;
diff --git a/cpp/demo/Ice/session/SessionFactoryI.h b/cpp/demo/Ice/session/SessionFactoryI.h
index c46c11c1b2d..28db4215fd4 100755
--- a/cpp/demo/Ice/session/SessionFactoryI.h
+++ b/cpp/demo/Ice/session/SessionFactoryI.h
@@ -10,18 +10,14 @@
 #ifndef SESSION_FACTORY_I_H
 #define SESSION_FACTORY_I_H
 
-// XXX Missing #includes, see comment in SessionI.h.
+#include <Ice/Ice.h>
 #include <Session.h>
 
-class SessionFactoryI : public Demo::SessionFactory, public IceUtil::Mutex // XXX What is the mutex needed for?
+class SessionFactoryI : public Demo::SessionFactory
 {
 public:
 
-    // XXX Constructor and destructor do nothing, get rid of them.
-    SessionFactoryI();
-    virtual ~SessionFactoryI();
-
-    virtual Demo::SessionPrx create(const Ice::Current&);
+    virtual Demo::SessionPrx create(const std::string&, const Ice::Current&);
     virtual void shutdown(const Ice::Current&);
 };
 
diff --git a/cpp/demo/Ice/session/SessionI.cpp b/cpp/demo/Ice/session/SessionI.cpp
index 803c5530036..36643f949dd 100755
--- a/cpp/demo/Ice/session/SessionI.cpp
+++ b/cpp/demo/Ice/session/SessionI.cpp
@@ -7,8 +7,6 @@
 //
 // **********************************************************************
 
-#include <Ice/Ice.h> // XXX Remove, see comment in SessionFactoryI.cpp
-#include <SessionFactoryI.h> // XXX Remove, not needed anywhere.
 #include <SessionI.h>
 
 using namespace std;
@@ -18,24 +16,26 @@ class HelloI : public Hello
 {
 public:
 
-    HelloI(int id) :
+    HelloI(const string& name, int id) :
+	_name(name),
 	_id(id)
     {
     }
 
     virtual ~HelloI()
     {
-	cout << "Hello object #" << _id << " destroyed" << endl;
+	cout << "Hello object " << _id << " for session " << _name << " destroyed" << endl;
     }
 
     void
     sayHello(const Ice::Current&) const
     {
-	cout << "Hello object #" << _id << " says 'Hello World!'" << endl;
+	cout << "Hello object " << _id << " says 'Hello " << _name << "!'" << endl;
     }
 
 private:
 
+    const string _name;
     const int _id;
 };
 
@@ -48,7 +48,7 @@ SessionI::createHello(const Ice::Current& c)
 	throw Ice::ObjectNotExistException(__FILE__, __LINE__);
     }
 
-    HelloPrx hello = HelloPrx::uncheckedCast(c.adapter->addWithUUID(new HelloI(_nextId++)));
+    HelloPrx hello = HelloPrx::uncheckedCast(c.adapter->addWithUUID(new HelloI(_name, _nextId++)));
     _objs.push_back(hello);
     return hello;
 }
@@ -65,6 +65,18 @@ SessionI::refresh(const Ice::Current& c)
     _timestamp = IceUtil::Time::now();
 }
 
+string
+SessionI::getName(const Ice::Current&) const
+{
+    Lock sync(*this);
+    if(_destroy)
+    {
+	throw Ice::ObjectNotExistException(__FILE__, __LINE__);
+    }
+
+    return _name;
+}
+
 void
 SessionI::destroy(const Ice::Current& c)
 {
@@ -76,8 +88,7 @@ SessionI::destroy(const Ice::Current& c)
 
     _destroy = true;
 
-    // XXX Why "#"? Use "The session with the identity `...' is now destroyed."
-    cout << "The session #" << Ice::identityToString(c.id) << " is now destroyed." << endl;
+    cout << "The session " << _name << " is now destroyed." << endl;
     try
     {
 	c.adapter->remove(c.id);
@@ -106,9 +117,11 @@ SessionI::timestamp() const
     return _timestamp;
 }
 
-SessionI::SessionI() :
+SessionI::SessionI(const string& name) :
+    _name(name),
     _timestamp(IceUtil::Time::now()),
     _nextId(0),
     _destroy(false)
 {
+    cout << "The session " << _name << " is now created." << endl;
 }
diff --git a/cpp/demo/Ice/session/SessionI.h b/cpp/demo/Ice/session/SessionI.h
index 30f5b923832..abe4eb73b6b 100755
--- a/cpp/demo/Ice/session/SessionI.h
+++ b/cpp/demo/Ice/session/SessionI.h
@@ -10,11 +10,8 @@
 #ifndef SESSION_I_H
 #define SESSION_I_H
 
-// XXX Missing #includes. The #includes for the header must be
-// self-contained, i.e. there must be no problem if you just do
-// #include<SessionFactoryI.h> from an empty .cpp file. (I know that
-// some other demos don't follow this rule, but they need to be
-// fixed. We shouldn't propagate such mistakes into new demos.)
+#include <Ice/Ice.h>

+#include <IceUtil/StaticMutex.h>
 #include <Session.h>
 #include <list>
 
@@ -25,6 +22,7 @@ public:
     virtual Demo::HelloPrx createHello(const Ice::Current&);
     virtual void refresh(const Ice::Current&);
     virtual void destroy(const Ice::Current&);
+    virtual std::string getName(const Ice::Current&) const;
 
 private:
 
@@ -34,12 +32,25 @@ private:
 
     // Only the session factory can create sessions.
     friend class SessionFactoryI;
-    SessionI();
+    SessionI(const std::string&);
 
+    const std::string _name;
     IceUtil::Time _timestamp; // The last time the session was refreshed.
 
-    // XXX This needs to be a static, otherwise hello objects from different client have the same ID.
-    int _nextId; // The id of the next hello object. This is used for tracing purposes.
+    //
+    // XXX This needs to be a static, otherwise hello objects from
+    // different client have the same ID.
+    //
+    // The reason I didn't make the _nextId static in the first place
+    // was because the client also displays the index of the hello
+    // object. I think displaying a different id on the client &
+    // server will be confusing, and I didn't think it was worth
+    // making the client keep a real id->hello object client map
+    // because a) it would make the client more complicated and b) it
+    // would require the user to type bigger and bigger numbers all
+    // the time which is a pain in the ass.
+    //
+    int _nextId; // The per-session 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;
 };