Skip to main content
Code Review

Return to Answer

Added clarification
Source Link
forsvarir
forsvarir
  • 11.7k
  • 6
  • 38
  • 71

remove'remove' naming

If there are no items in any of the queues (such as when the thread is initially created) then the thread function will spin, draining resources that could be used on other things such as inserting items into a queue. One approach to improve this would be to count items managed and wait while it is zero (there are other strategies, or use a single flag that could be more efficient)is set when items are inserted and have the thread wait on it if it fails to read from any of the queues.

If this is called twice you will start multiple worked threads. Is this expected/desired behaviour? I would assume not, in which case you should add protection to stop a second thread being started. One approach might be to make the (if there are multiple removeinitWorkerThread private and have it called automatically from the constructor. With the current approach if two threads are started, there is a race condition between checking the queues size and removing an item)? If not you should add protection to stop a second thread being started.

void QueuesManager::remove(void) {
while(true) {
    info_conn ic;
    if (maxQ.size() > 0) {  // <-- If two threads check this at the same 
                            // time and pass, but there is only 1 item
                            // in the queue
        std::unique_lock<std::mutex> lck(m1);
                            // One thread will make it through the wait
                            // condition, whilst the other will be stuck
                            // waiting for another item to be inserted
                            // into the queue
        r1.wait(lck, [this] () { return maxQ.size() > 0; });

These feel wrong. If you only have one worker removing items from the queue and you are confident about sizesize being thread safe then they are redundant, the. The if statement ensures the condition isyou are going to wait on has already been met. If The mutex can simply be locked.

if (maxQ.size() > 0) {  // <- This
    std::unique_lock<std::mutex> lck(m1);
    r1.wait(lck, [this] () { return maxQ.size() > 0; }); // <- And this
                                                         // are the same check

If either of these isn't true and the remove thread actually blocks on these variables, it is because the thread is blocked on an empty queue that it expected to have an item (which seems like a bug). It will wait inan indeterminate amount of time during which it isn't going to service other queues that might have items in them.

remove naming

If there are no items in any of the queues then the thread function will spin, draining resources that could be used on other things such as inserting into a queue. One approach to improve this would be to count items managed and wait while it is zero (there are other strategies that could be more efficient).

If this is called twice you will start multiple worked threads. Is this expected/desired behaviour (if there are multiple remove threads, there is a race condition between checking the queues size and removing an item)? If not you should add protection to stop a second thread being started.

These feel wrong. If you only have one worker and you are confident about size then they are redundant, the if statement ensures the condition is already met. If either of these isn't true and the remove thread actually blocks on these variables, it is because the thread is blocked on an empty queue that it expected to have an item. It will wait in indeterminate amount of time during which it isn't going to service other queues that might have items in them.

'remove' naming

If there are no items in any of the queues (such as when the thread is initially created) then the thread function will spin, draining resources that could be used on other things such as inserting items into a queue. One approach to improve this would be to count items managed and wait while it is zero, or use a single flag that is set when items are inserted and have the thread wait on it if it fails to read from any of the queues.

If this is called twice you will start multiple worked threads. Is this expected/desired behaviour? I would assume not, in which case you should add protection to stop a second thread being started. One approach might be to make the initWorkerThread private and have it called automatically from the constructor. With the current approach if two threads are started, there is a race condition between checking the queues size and removing an item.

void QueuesManager::remove(void) {
while(true) {
    info_conn ic;
    if (maxQ.size() > 0) {  // <-- If two threads check this at the same 
                            // time and pass, but there is only 1 item
                            // in the queue
        std::unique_lock<std::mutex> lck(m1);
                            // One thread will make it through the wait
                            // condition, whilst the other will be stuck
                            // waiting for another item to be inserted
                            // into the queue
        r1.wait(lck, [this] () { return maxQ.size() > 0; });

These feel wrong. If you only have one worker removing items from the queue and you are confident about size being thread safe then they are redundant. The if statement ensures the condition you are going to wait on has already been met. The mutex can simply be locked.

if (maxQ.size() > 0) {  // <- This
    std::unique_lock<std::mutex> lck(m1);
    r1.wait(lck, [this] () { return maxQ.size() > 0; }); // <- And this
                                                         // are the same check

If either of these isn't true and the remove thread actually blocks on these variables, it is because the thread is blocked on an empty queue that it expected to have an item (which seems like a bug). It will wait an indeterminate amount of time during which it isn't going to service other queues that might have items in them.

Source Link
forsvarir
forsvarir
  • 11.7k
  • 6
  • 38
  • 71

Don't silently fail

This should probably throw an exception, swallowing errors like this leads to hidden bugs:

default: {
    std::cout << "You shouldn't be here\n" << std::flush;
    break;
}

remove naming

From the class header it is not obvious that this is going to be a never exit thread worker method. I would be concerned that a future client might try to call it to remove one item from a queue. Consider renaming it and making it private.

Worker thread

Your worker thread has no termination condition. You should provide a mechanism to signal it that processing is complete and it is time to terminate.

If there are no items in any of the queues then the thread function will spin, draining resources that could be used on other things such as inserting into a queue. One approach to improve this would be to count items managed and wait while it is zero (there are other strategies that could be more efficient).

initWorkedThread

If this is called twice you will start multiple worked threads. Is this expected/desired behaviour (if there are multiple remove threads, there is a race condition between checking the queues size and removing an item)? If not you should add protection to stop a second thread being started.

size

I don't like that you are calling size outside of the guard methods. Is it documented that it is thread safe to do so? It probably won't cause an issue, since you check again within a guard, but it feels like you are making assumptions about how it is implemented.

r1 to 4

These feel wrong. If you only have one worker and you are confident about size then they are redundant, the if statement ensures the condition is already met. If either of these isn't true and the remove thread actually blocks on these variables, it is because the thread is blocked on an empty queue that it expected to have an item. It will wait in indeterminate amount of time during which it isn't going to service other queues that might have items in them.