3
\$\begingroup\$

I have a basic websocket server implemented in C++. I'm using Intel TBB's concurrent hash_map to store all the users, uniquely identified by the id. The concurrent hash map will hold a key value pair of <int> and <std::set<string>> where the int is the user's id, and the <std::set<string>> is a set of all instances of the user. In this example, I just used string, but in production, it'll be a websocket object such as uWS::WebSocket<uWS::SERVER>*.

When the user connects, we add it to the hash map: if the user is already in the hash_map, then it should just add the user instance in the user's corresponding value <set>. If the user is not in the hash_map, then we create a new pair <int, set> and add the user.

I've attached my full code here. I'm expecting about 50,000 users at any given time connected. I have two questions:

  • Is my code thread safe? When I executed it on multiple threads, I didn't find any race conditions or deadlocks, but that doesn't mean there aren't any.
  • Can you give any suggestions on how to improve my code, both performance and maintainability wise?

#include <iostream>
#include <set>

// intel tbb 
#include "tbb/concurrent_hash_map.h"

typedef std::string client;
typedef std::set<client*> set;
typedef tbb::concurrent_hash_map<int, set*> hash_map;

// return set* on found, otherwise null
set* find(hash_map* users, int key){
    tbb::concurrent_hash_map<int, set*>::const_accessor ac;
    bool found = users->find(ac, key);

    set* returnValue;

    if (found){
        returnValue = ac->second;
    }
    else {
        returnValue = NULL;
    }

    ac.release();
    return returnValue;
}

/*
    insert a key-value pair to the hash_map
    if the key is found already, then there must already be a set* as a value
        in this case, just insert the client* into the set
    if the key is not found
        create a new set*
        insert the user to this set*
        insert a new user in the hash map with the key
        assign the value of this key in the hash map to the set*
*/
void insert(hash_map* users, int key, client* user){
    tbb::concurrent_hash_map<int, set*>::accessor ac;

    set* list_of_users = find(users, key);
    if (list_of_users == NULL){
        set* v = new set();
        v->insert(user);

        users->insert(ac, key);
        ac->second = v;

    }
    else {
        // not null
        list_of_users->insert(user);
    }

    ac.release();
}

/*
    remove a client* from the set*;
    after insertion, if the set* is of size 0
        delete set*
        remove key from hash_map
    otherwise
        just remove the client* from the uer
*/
void remove(hash_map* users, int key, client* user){
    set* list_of_users = find(users, key);

    if (list_of_users == NULL){
        return; // hash map does not contain the key, so can't remove
    }

    tbb::concurrent_hash_map<int, set*>::accessor ac;
    list_of_users->erase(user);
    if (list_of_users->empty()){
        // no more users
        std::cout << "no more elements in set, deleting it all " << std::endl;
        delete list_of_users;
        users->erase(key);
    }
    ac.release();
}

int main() {
    hash_map* users = new hash_map();

    // create the users
    std::string user1_pc = "user 1 pc";
    std::string user1_mobile = "user 1 mobile";
    std::string user1_tablet = "user 1 tablet";

    std::string user2_mobile = "user 2 mobile";
    std::string user2_pc = "user 2 pc";

    std::string user3_pc = "user 3 pc";

    // insert the users
    insert(users, 1, &user1_pc);
    insert(users, 1, &user1_mobile);
    insert(users, 1, &user1_tablet);

    insert(users, 2, &user2_mobile);
    insert(users, 2, &user2_pc);

    insert(users, 3, &user3_pc);

    // remove the users
    remove(users, 3, &user3_pc);

    remove(users, 2, &user2_mobile);
    remove(users, 2, &user2_pc);

    remove(users, 1, &user1_pc);
    remove(users, 1, &user1_mobile);
    remove(users, 1, &user1_tablet);

    /*
        no more elements in set, deleting it all 
        no more elements in set, deleting it all 
        no more elements in set, deleting it all 
    */
    return 0;
}
\$\endgroup\$
2
\$\begingroup\$

C Style

Your code looks more like c than c++, which might be fine for what you are trying to achieve but in general i'd like to see more abstraction and encapsulation.

C++11

Unless your environment is very constrained with regard to the use of c+11 (and up) there is no reason not to use smart pointers, std::unique_ptr and std::shared_ptr actually help writing multithreaded code as ownership of dynamically allocated classes becomes explicit. Use nullptr instead of NULL

typedef for shortcuts

It's a little bit minor but if you are already creating typedefs for certain types you might as well give the more expressive names. WHAT is it that is being defined Usertable, etc ...

Comment your business case not the functionality that you are executing

Your comments paraphrase the code that is in the functions they mix a little bit of the functions that are being executed with some of the domain language that you are using "User" and "Connection" for example. But I really can't quite fathom what you are tracking and how. e.g. you use key a key is literally anything, connectionId or userId would make everything more understandable. Having a correct encapsulation class ConnectionTable, or whatever the hash_map is supposed to be would help.

Comment what can't be seen in the code

// return set* on found, otherwise null is an almost useless comment I can see that from the code. Comment your assumptions coming in, comment in language of your domain. Don't comment the obvious

Use Multithreaded tests

If you want to test whether code is correct under multithreaded conditions you have to test using multiple thread, the test code that you posted here doesn't reflect that, while it seems to verify the single threaded behavior of your code, it doesn't test multithreaded behavior. With that said correctly running multithreaded tests don't necessarily mean that the code is bug free. You said you have these tests without looking at the code we can't tell you if they are valid or not

Not threadsafe

Your code is not threadsafe, in remove you don't seem to acquire the accessor to the find result, this means that the access to the result pair would not be threadsafe with regard to the other functions accessing that pair. Any operations on list_of_users is not threadsafe at that time.

You also return set* from your find function, any user of find can now modify the set at will breaking thread safety. At least returning const set* would prevent callers from modifying the result. But access to set could still collide with changes to the set only returning a copy at this point would prevent problems.

Improvements

Looking at the code and your description I still can't quite fathom what you are collecting where, a proper interface with names corresponding to the operations and objects that you are working with would help. Encapsulating everything in an appropriate class is also an option. E.g. find is that findUsers, findConnectionsForUser or something else ... ?? same with all the other functions.

\$\endgroup\$
  • \$\begingroup\$ Excellent, thorough code review. \$\endgroup\$ – Contango 3 hours ago

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.