Make most operations on Server deferred, so that we a) do not get bugs with epoll...

diff --git a/server.cpp b/server.cpp
index d8c68fc4f5117df388d9d9c852f351ce92f8d330..60836a74ca52774855f80c2a3ce623545dbc534f 100644 (file)
--- a/server.cpp
+++ b/server.cpp
@@ -109,6 +109,7 @@ void Stream::wake_up_all_clients()
 Server::Server()
 {
        pthread_mutex_init(&mutex, NULL);
 Server::Server()
 {
        pthread_mutex_init(&mutex, NULL);

+       pthread_mutex_init(&queued_data_mutex, NULL);

 
        epoll_fd = epoll_create(1024);  // Size argument is ignored.
        if (epoll_fd == -1) {
 
        epoll_fd = epoll_create(1024);  // Size argument is ignored.
        if (epoll_fd == -1) {


@@ -178,6 +179,8 @@ void Server::do_work()
                        return;
                }
 
                        return;
                }
 

+               process_queued_data();
+

                for (int i = 0; i < nfds; ++i) {
                        int fd = events[i].data.fd;
                        assert(clients.count(fd) != 0);
                for (int i = 0; i < nfds; ++i) {
                        int fd = events[i].data.fd;
                        assert(clients.count(fd) != 0);


@@ -203,8 +206,11 @@ void Server::do_work()
        }
 }
 
        }
 }
 

-CubemapStateProto Server::serialize() const
+CubemapStateProto Server::serialize()

 {
 {

+       // We don't serialize anything queued, so empty the queues.
+       process_queued_data();
+

        CubemapStateProto serialized;
        for (map<int, Client>::const_iterator client_it = clients.begin();
             client_it != clients.end();
        CubemapStateProto serialized;
        for (map<int, Client>::const_iterator client_it = clients.begin();
             client_it != clients.end();


@@ -219,9 +225,14 @@ CubemapStateProto Server::serialize() const
        return serialized;
 }
 
        return serialized;
 }
 

+void Server::add_client_deferred(int sock)
+{
+       MutexLock lock(&queued_data_mutex);
+       queued_add_clients.push_back(sock);
+}
+

 void Server::add_client(int sock)
 {
 void Server::add_client(int sock)
 {

-       MutexLock lock(&mutex);

        clients.insert(make_pair(sock, Client(sock)));
 
        // Start listening on data from this socket.
        clients.insert(make_pair(sock, Client(sock)));
 
        // Start listening on data from this socket.


@@ -297,14 +308,15 @@ void Server::set_header(const string &stream_id, const string &header)
                }
        }
 }
                }
        }
 }

-       
-void Server::add_data(const string &stream_id, const char *data, size_t bytes)
+
+void Server::add_data_deferred(const string &stream_id, const char *data, size_t bytes)

 {
 {

-       if (bytes == 0) {
-               return;
-       }
+       MutexLock lock(&queued_data_mutex);
+       queued_data[stream_id].append(string(data, data + bytes));
+}

 
 

-       MutexLock lock(&mutex);
+void Server::add_data(const string &stream_id, const char *data, size_t bytes)
+{

        Stream *stream = find_stream(stream_id);
        size_t pos = stream->data_size % BACKLOG_SIZE;
        stream->data_size += bytes;
        Stream *stream = find_stream(stream_id);
        size_t pos = stream->data_size % BACKLOG_SIZE;
        stream->data_size += bytes;


@@ -608,3 +620,20 @@ Stream *Server::find_stream(const string &stream_id)
        assert(it != streams.end());
        return it->second;
 }
        assert(it != streams.end());
        return it->second;
 }

+
+void Server::process_queued_data()
+{
+       MutexLock lock(&queued_data_mutex);
+
+       for (size_t i = 0; i < queued_add_clients.size(); ++i) {
+               add_client(queued_add_clients[i]);
+       }
+       queued_add_clients.clear();     
+       
+       for (map<string, string>::iterator queued_it = queued_data.begin();
+            queued_it != queued_data.end();
+            ++queued_it) {
+               add_data(queued_it->first, queued_it->second.data(), queued_it->second.size());
+       }
+       queued_data.clear();
+}

diff --git a/server.h b/server.h
index d87e7dfab0d8dd9a08423b7b2c47b998e2b277a2..ce9365d7a49cfb98d176d9a566358c2a5f98d937 100644 (file)
--- a/server.h
+++ b/server.h
@@ -105,21 +105,43 @@ public:
 
        // Stop the thread.
        void stop();
 
        // Stop the thread.
        void stop();

+       
+       void set_header(const std::string &stream_id, const std::string &header);

 
 

-       CubemapStateProto serialize() const;
+       // These will be deferred until the next time an iteration in do_work() happens,
+       // and the order between them are undefined.
+       // XXX: header should ideally be ordered with respect to data.
+       void add_client_deferred(int sock);
+       void add_data_deferred(const std::string &stream_id, const char *data, size_t bytes);

 
 

-       void add_client(int sock);
+       // These should not be called while running, since that would violate
+       // threading assumptions (ie., that epoll is only called from one thread
+       // at the same time).
+       CubemapStateProto serialize();

        void add_client_from_serialized(const ClientProto &client);
        void add_client_from_serialized(const ClientProto &client);

-

        void add_stream(const std::string &stream_id);
        void add_stream_from_serialized(const StreamProto &stream);
 
        void add_stream(const std::string &stream_id);
        void add_stream_from_serialized(const StreamProto &stream);
 

-       void set_header(const std::string &stream_id, const std::string &header);
-       void add_data(const std::string &stream_id, const char *data, size_t bytes);
-

 private:
        pthread_t worker_thread;
 
 private:
        pthread_t worker_thread;
 

+       // Mutex protecting queued_data only. Note that if you want to hold both this
+       // and <mutex> below, you will need to take <mutex> before this one.
+       pthread_mutex_t queued_data_mutex;
+
+       // Deferred commands that should be run from the do_work() thread as soon as possible.
+       // We defer these for two reasons:
+       //
+       //  - We only want to fiddle with epoll from one thread at any given time,
+       //    and doing add_client() from the acceptor thread would violate that.
+       //  - We don't want the input thread(s) hanging on <mutex> when doing
+       //    add_data(), since they want to do add_data() rather often, and <mutex>
+       //    can be taken a lot of the time.
+       //      
+       // Protected by <queued_data_mutex>.
+       std::vector<int> queued_add_clients;
+       std::map<std::string, std::string> queued_data;
+

        // All variables below this line are protected by the mutex.
        pthread_mutex_t mutex;
 
        // All variables below this line are protected by the mutex.
        pthread_mutex_t mutex;
 


@@ -173,6 +195,11 @@ private:
 
        // TODO: This function should probably die.
        Stream *find_stream(const std::string &stream_id);
 
        // TODO: This function should probably die.
        Stream *find_stream(const std::string &stream_id);

+
+       void process_queued_data();
+
+       void add_client(int sock);
+       void add_data(const std::string &stream_id, const char *data, size_t bytes);

 };
 
 #endif  // !defined(_SERVER_H)
 };
 
 #endif  // !defined(_SERVER_H)

diff --git a/serverpool.cpp b/serverpool.cpp
index f8fece60a43c0a17d303c74dbd48e9e5a986d695..a2035943d429a53e1c621ab6c773b08fdac56994 100644 (file)
--- a/serverpool.cpp
+++ b/serverpool.cpp
@@ -16,7 +16,7 @@ ServerPool::~ServerPool()
 
 void ServerPool::add_client(int sock)
 {
 
 void ServerPool::add_client(int sock)
 {

-       servers[clients_added++ % num_servers].add_client(sock);
+       servers[clients_added++ % num_servers].add_client_deferred(sock);

 }
 
 void ServerPool::add_client_from_serialized(const ClientProto &client)
 }
 
 void ServerPool::add_client_from_serialized(const ClientProto &client)


@@ -48,7 +48,7 @@ void ServerPool::set_header(const std::string &stream_id, const std::string &hea
 void ServerPool::add_data(const std::string &stream_id, const char *data, size_t bytes)
 {
        for (int i = 0; i < num_servers; ++i) {
 void ServerPool::add_data(const std::string &stream_id, const char *data, size_t bytes)
 {
        for (int i = 0; i < num_servers; ++i) {

-               servers[i].add_data(stream_id, data, bytes);
+               servers[i].add_data_deferred(stream_id, data, bytes);

        }
 }
 
        }
 }