People who code: we want your input. Take the Survey
29

Prefer Reference over Pointer Since the producer and consumer must have a buffer you should pass it by reference (rather than pointer). This also makes sure there is no confusion over ownership of the buffer (the owner of a pointer is responsible for deleting it). By using a RAW pointer you can not tell the owner but by using a reference you are explicitly ...


15

I have a few comments about your code: Using std::lock_guard is great to handle mutexes since it automatically unlocks the acquired mutex when leaving the scope. That's a real great tool. You should really use it everywhere you can. Using it consistently will make sure that you can't forget to unlock any mutex. Moreover, it makes sure that mutexes are ...


11

Style Standard C# naming convention for methods is PascalCase. For static and instance members there are more variants around but often they are prefixed with _ and/or area also PascalCase so they can be easily distinguished from local variables and parameters. Following standard naming conventions makes your code look more familiar to other C# developers. ...


11

Design The problem with your implementation of pop() is that it can not be implemented in an exception safe manner (for any type of T). This is why the standard implementation implements this as two different methods top() and pop(). The top() function simply returns the value while pop() does not return the value but simply removes the value from head. So ...


10

static int avail = 0; ... while (avail == buffSize) { }; ... avail++; You can't just access the same field from multiple threads at the same time and assume it will work correctly. As already pointed out by ChrisWue, incrementing a value is not an atomic operation, and you could fix that by using Interlocked.Increment() instead. Another problem is ...


10

First of all it would be good to use the standard Java Conventions, i.e. method names start with a lower case letter. i.e. StartconsumerProducer shold either be startconsumerProducer or simply run. Also handle InterruptedException correctly. For details have a look at the book Java Concurrency in Practice In your case it does not make sense to catch the ...


7

Issues: Failed to check the return value of all these system calls: stack->storage = malloc(sizeof(void*) * stack->capacity); pthread_mutex_init(&stack->mutex, NULL); pthread_cond_init(&stack->empty_condition_variable, NULL); pthread_cond_init(&stack->full_condition_variable, NULL); pthread_mutex_lock(&stack->mutex); ...


7

The implementation clearly has a race condition: Let's assume Count == _boundedCapacity - 1 Now have 2 producers call Insert simultaneously Both simultaneously check that the current size is less than the upper bound and move beyond the while loop Both insert an item. Now you've exceeded the bounded capacity by 1 While it might be ok to be able to exceed ...


6

#include <condition_variable> std::condition_variable cv; #include <iostream> #include <random> #include <mutex> std::mutex mtx; #include <thread> Don't mix includes and code, makes it easy to miss those globals you're defining in the middle. Keep the headers by themselves sorted in some order or grouped logically/by ...


6

Your consumer thread locks out the producer for extended periods of time. void ConsumeData() { while (true) { std::unique_lock<std::mutex> uniqueLock(mutex); conditionVariable.wait(uniqueLock, [] {return !queue.empty(); }); while (!queue.empty()) { // DO WORK queue.pop(); } uniqueLock.unlock(); } } I ...


6

This is sort of an extended comment on Martin York's reply. When you're doing any sort of parallel processing, I advise against re-designing pop so it requires two operations to actually remove an item from the queue, like: T val = queue.top(); queue.pop(); With sufficient care, this can work for a single-producer/single-consumer situation, but has the ...


6

I get a couple of warnings, which ought to be fixed: g++ -std=c++2a -fPIC -g -Wall -Wextra -Wwrite-strings -Wno-parentheses -Wpedantic -Warray-bounds -Weffc++ -pthread 235651.cpp -o 235651 235651.cpp: In constructor ‘Buffer::Buffer()’: 235651.cpp:31:5: warning: ‘Buffer::mu’ should be initialized in the member initialization list [-Weffc++] 31 | ...


6

I am confused about the design. Normally one reuses the same thread instead of creating one for each minor task. Thread creation is not cheap on most platforms. 1. void give(Input&& resource) will be clunky to use as input is an r-value reference which is inconvenient. In many template functions you see && used a lot but there it is most ...


5

Did you know that Task.Result blocks? Your code isn't asynchronous at all - and it can't be unless DoRead is. Effectively your code is starting a new task and then blocking while it completes - there's no benefit to starting the task at all! As a small aside, Task.Run is easier than using Task.Factory.StartNew. If I were you, I'd just remove the Task.


5

You implemented your producers and consumers as Runnable -- that's good. You are also, judging from your sample output, using a thread naming mechanism that makes it easier to identify what's going on -- that's very good. Not bad. Buckle up. There are a couple immediate problems with your producer logic. First, your producers are generating identical ...


5

Producer and Consumer You properly synchronize on the sharedList. Both Producer and Consumer fail to use the proper wait() idiom to guard against spurious wake up. Producer waits until the list is empty : while(!sharedList.isEmpty()) sharedList.wait(); Consumer waits until the list is not empty : while(sharedList.isEmpty()) sharedList.wait(); ...


5

When calling the function malloc(): The returned type is void*, so it can be assigned to any pointer, making the 'cast' totally unneeded. It just clutters the code, making it more difficult to understand, debug, maintain. The expression sizeof(char) is defined in the standard at 1. Multiplying anything by 1 has no effect, so the expression just clutters the ...


5

Dead Lock Bug If the queue is empty and the consumer thread is waiting in the condition variable. If you Queue goes out of scope the main thread will enter the detructor and perform join(). At this point you are locked. As both threads are waiting for the other. Use of condition can be simplified. while(queue.empty()) { cv.wait(lock); }...


5

Buffer<T> This class seems fine for the most part. Some small issues: The while(true) loops in insert(T) and remove() only ever run one iteration. Those loops can be replaced by their body. add calls m_buffer.push_back(x), which might not compile for types without a copy constructor. Try m_buffer.push_back(std::move(x)) instead: That will move if ...


5

You have comments in the definition of RingBuffer on the value limits for some members, but there is nothing in the code that enforces that or checks to see if those limits are exceeded. Why does TryRead take its parameter by const int&? The reference is unnecessary. Just pass in the int. Why is MAX_SIZE a macro? It should be a constexpr (if your ...


5

In addition to what has already been said I would add that what happens if s is zero? You might want to throw an exception in the constructor if that happens probably because any call to count of room would fail otherwise. you can definitely improve const correctness at least in a couple of places: in explicit PCQueue(size_t s) : you can make s const. ...


5

Avoid mixing clock types Why use std::chrono::_V2::system_clock::time_point start but initialize it with std::chrono::high_resolution_clock::now()? If there is a reason to use the non-standard _V2 clocks, then you should probably stick with it everywhere. But if there is no reason to, avoid the non-standard _V2 thing. To make your code more consistent and ...


4

First of all: your code is not bad; I agree with Wayne Conrad. I have a minor remark about the connect statement. You are currently using Qt 4.8, so this is fine as it is. However with Qt 5 a new signal/slot syntax was introduced. Your question about threading: Don't be afraid of QThread, they're just threads too. The problem with QThread is only this: a ...


4

Just a few nit-picky things: You are using C++, so it would be wise to split up your code into a header and source file. Showing your include statements would be helpful. I see no documentation on your methods. Unless it's something very straightforward (like a programming contest entry), methods in your public code should generally have some form of ...


4

Static class As proposed by eurotrash I would not implement the class as static class because it has some disadvantages: It is only possible to use one class per application It is not possible to mock the class (e.g. for unit test / alternative implementations) It is not possible to use it with DI. It is not very common to have a statefull static classes - ...


4

Below are my suggestions grouped by class and the code including these suggestions at the end. CircularBuffer I would advice using an implementation of BlockingQueue (for your case ArrayBlockingQueue seems appropriate). Nevertheless, below are my suggestions to improve this class : Use a specific variable for the number of available elements You know the ...


4

Extraneous line I quickly scanned your stack code and it looked correct to me. However, I did spot an extraneous line in concurrent_stack_top(). You have this line that was probably copied from concurrent_stack_pop(): pthread_cond_signal(&stack->full_condition_variable); Since concurrent_stack_top() doesn't remove anything from the stack, you ...


4

Actually, the initial producer-consumer scheme is over-complicated and has the following issues: Using 2 separate threading.Semaphore objects: One of them is used for producer (remaining_space), another - for consumer (fill_count). Though they won't be convenient here (as I'll describe below), but for your future potential cases, a better naming would be ...


4

std::this_thread::sleep_for(std::chrono::milliseconds(50)); Don't do this. Use a condition_variable instead. This will require some minor revising. 50ms might be a long time. Remember that to the OS, it means "context switch out the thread and keep it idle for at least 50ms". A std::mutex may have all manner of fancy implementation dependent optimizations. ...


3

Missing barrier When I looked at your code, I could immediately tell there was something wrong because you use a release barrier without an acquire barrier that pairs with it. You have this barrier in try_enqueue() between writing to m_data and writing to m_tail_2: std::atomic_thread_fence( std::memory_order_release ); But you don't have any corresponding ...


Only top voted, non community-wiki answers of a minimum length are eligible