25

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 ...


14

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. ...


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 ...


10

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 ...


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

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 | ...


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

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

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

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

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 ...


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

As a matter of being thread-safe, you should replace this: private void TimerOnElapsed(object sender, ElapsedEventArgs e) { if (BatchAvailable != null) BatchAvailable(sender , e); } with this: private void TimerOnElapsed(object sender, ElapsedEventArgs e) { var batchAvailable = this.BatchAvailable; ...


4

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(); ...


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

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

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 ...


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

Queue queue = new Queue(10); servicePool = new ServiceThreadPool(10,queue); requestPool = new RequestThreadPool(1000,queue); You should replace those numbers with constants. Reading that I'm not sure -- is the ServiceThreadPool the same size as the queue? Or is that a coincidence? With constants, it would be obvious. An ...


3

I don't really understand how you want to use this so I'm just going to add a couple of comments for you to ponder. Naming It's more common to abbreviate Asynchronous to Async not Asynch when used in identifers: AsynchSimpleProducer<T> -> AsyncSimpleProducer<T>. NewItem isn't a great name for an event, events generally need a verb to make ...


3

Robust code does not assume the first char in buf is not '\0'. This is a pedantic point, but user IO is a hacker paradise - best to code defensively. Instead of: size_t str_len; while (fgets(buf, sizeof(buf), fp) != NULL) { str_len = strlen(buf); if(buf[str_len - 1] != '\n') printf("Didn't read whole line!\n"); else { buf[str_len - 1] = '\0';...


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