views:

138

answers:

4

Hello, I have been trying to create a TCP Server model based on inheritance, with varying success. These servers are managed by a singleton whose task it is to shut these servers down and other simple maintenance functions:

class TCPServer {
public:
    TCPServer();
    ~TCPServer();

    void Bind(TCPDaemon *daemon) {
        if(!daemon->IsRunning()) {
            throw TCPBindException("Daemon is inactive");
        }

        // if the port is not taken, bind this daemon to it
        if(this->servers.count(daemon->port())==0) {
            this->servers[daemon->port()]=daemon;
            ...
        } else {
            throw TCPBindException("Port is taken");
        }
    }

    void Shutdown() {
        MASON::UINT16 i;
        for(i=0;i<this->servers.size();i++) {
            this->Shutdown((*this->servers.begin()).first);
        }
    }

    void Shutdown(unsigned short port)  {
        if(this->servers.count(port)) {

            if(this->servers[port]->IsRunning()) {
                this->servers[port]->Stop();
            }

            delete this->servers[port];
            this->servers.erase(port);

        }
    }

private:
    std::map<unsigned short, TCPDaemon*> servers;

};

The TCPDaemon class's Stop() function is a pure virtual. My problem is that when the Shutdown() function is called, it is attempting to call this pure virtual instead of the derived class' version. How can I force it to do the right thing?

Thanks in advance

[edit] sorry I did not include the TCPDaemon code before, it derives from a TCPSocket class (which I have checked to be 100% working and is fairly self-explanatory). Here it is:

class TCPDaemon: public TCPSocket {
public:
    TCPDaemon(unsigned short port) {
        this->_enabled=false;
        this->_host.ipaddr(INADDR_ANY);
        this->_host.port(port);
        this->paused=false;

        struct sockaddr_in opts=this->_host.Compile();

        #ifdef PLATFORM_WINDOWS
            WSADATA wsaData;
            if(WSAStartup(0x0202, &wsaData)) {
                throw TCPDaemonException("Failed to start WSA");
            }
        #endif

        this->raw_socket=socket(AF_INET, SOCK_STREAM, 0);
        if(this->raw_socket<=0) {
            throw TCPDaemonException("Failed to create socket");
        }

        if(int status=bind(this->raw_socket, (sockaddr*)&opts, sizeof(sockaddr))) {
            printf("error [%i]\r\n", status);
            throw TCPDaemonException("Failed to bind to port");
        }

        if(listen(this->raw_socket, 5)) {
            throw TCPDaemonException("Failed to listen on port");
        }

        this->_enabled=true;

    }

    virtual ~TCPDaemon() {
        this->Shutdown();
    }

    virtual void Start()=0;
    virtual void Run(TCPSocket*)=0;
    virtual void Stop()=0;

    unsigned short port() {
        return this->host().port();
    }

    bool IsRunning() {
        return this->_enabled;
    }

    TCPSocket *Accept() {
        SOCKET client;
        struct sockaddr client_addr;
        int len=sizeof(client_addr);
        client=accept(this->raw_socket, &client_addr, &len);

        return new TCPSocket(client, &client_addr);
    }

    void Shutdown() {

    }

private:
    bool _enabled;
    bool paused;

};

and here is a sample derived server and its method of creation:

   class EchoServer: public TCPDaemon {
    public:
        EchoServer(MASON::UINT16 port): TCPDaemon(port) {
        }

        ~EchoServer() {}

        virtual void Start() {

        }

        virtual void Run(TCPSocket *client) {
            printf("RUN\r\n");
            Accessor<TCPSocket> acc_client=client;
            acc_client->Write(Accessor<Blob> (new Blob(std::string("hello!"))));
            acc_client->Disconnect();
        }

        virtual void Stop() {

        }

    };

myTCPServer->Bind(new EchoServer(8008));

[edit+1] I think what the problem boils down to is this (i could easily be wrong): I have a std::map of the base class, TCPDaemon, which has a pure virtual/abstract function Stop(). It appears that when I call Stop() through one of the entries in the map, it is attempting to call TCPDaemon::Stop(), as opposed to the overriding function EchoServer::Stop(). Could this be the issue? If so, how do I resolve it?

A: 

If you say that it calls pure virtual method instead of derived class' version, that means that your derived class calls it. E.g.

class t1
{
 public:
virtual ~t1(){};
virtual void foo()=0        
  {
    std::cout << "pure virtual";
};
};

class t2:public t1
{
public :

virtual void foo() 
    {
    t1::foo();
    std::cout << "derived class ";
};
};

As far as I remember only derived class' object can call base class' pure virtual function (surely, if it's implemented in base class)

a1ex07
Or he just forgot `virtual` somewhere, or otherwise misused the term "pure virtual"
Potatoswatter
+1  A: 

Check the syntax of what you are declaring:

class TCPDaemon
{
    virtual void stop() = 0;
};

class MyDaemon : public TCPDaemon
{
    virtual void stop()
    {
        //Do stuff here.
    }
};

That's the best I can do without more code.

EDIT:

Ok, so it seems like you are using abstract functions. Next question is this: what is the error you are getting? I can say for certain that it is not attempting to call Stop() from TCPDeamon. That would be impossible, as it is not even implemented.

Stargazer712
Your code is correct, but you're incorrect that it's not a pure virtual function; in your code, TCPDaemon::stop *is* a pure virtual function. The OP's code is not.
Jacob
Seems like I had my terminology messed up. Thanks for the catch.
Stargazer712
A: 

I'm not sure if this is the problem you're seeing, but there's certainly a problem in the following function.

void Shutdown() {
    MASON::UINT16 i;
    for(i=0;i<this->servers.size();i++) {
        this->Shutdown((*this->servers.begin()).first);
    }
}

After each loop iterator, i will increase by one AND size will decrease, so you'll only ShutDown half your servers. Maybe keeping some open is the cause of further errors.

You could do either:

void Shutdown() {
    MASON::UINT16 i;
    std::size_t nrServers = this->servers.size();
    for(i=0;i<nrServers;i++) {
        this->Shutdown((*this->servers.begin()).first);
    }
}

or, which I would prefer as it better shows the intention of the code:

void Shutdown() {
    while (!this->servers.empty()) {
        this->Shutdown((*this->servers.begin()).first);
    }
}
Pieter
but i is not being used as an index - only a counter
pm100
@pm100 that's not relevant, it's used to determine when to exit the loop. Say you have 4 servers, after one loop iteration, `i` will be 1 and `size()` will be 3, after another, `i` will be 2 and `size()` will be 2. And the loop will terminate, leaving 2 servers 'alive'
Pieter
ur right - he should do while(!empty())
pm100
+1  A: 

I finally worked it out in the end, thanks to the input I have received. The problem was with the delete call in TCPServer::Shutdown(unsigned short), which was causing a memory access violation in a completely different part of the code... A rather noobie mistake, I shall be wrapping smart pointers around everything ASAP.

Thanks for all your feedback!

mrnoob
If you're calling Stop() from TCPDaemon::Shutdown(), and that's called from TCPDaemon::~TCPDaemon, then I'm pretty the vtbl will be torn down to where it tries to call TCPDaemon::Stop() and not your derived version. You have to be very careful what you call in a constructor or destructor, as you'll get *your class' version* of virtual functions.
kolbyjack
Thats what I was doing at first, a few hours before I posted this question, but through research I discovered the effect you have just explained and moved the Stop() into TCPServer for good measure. A lesson learned the hard way for sure!
mrnoob
btw - if u r noob then nice code. Look at using boost_shared_ptr instead of messing with raw pointers. Also - why not do this in c#, much less painful
pm100