Hot answers tagged

10

Things you did well Nicely formatted, easy to read. Use of typedef with structures. Things you could improve Preprocessor: Since SDL.h isn't one of your own pre-defined header files, you should be searching for it in directories pre-designated by the compiler (since that is where it should be stored). #include <SDL/SDL.h> In the C standard, §6.10....


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


8

General comments The node structure is defined as a struct with private fields. This makes it a class. If it has private fields, prefer a class. The node structure is an implementation detail that is not exposed, you do not need to bother with private here. Just remove the friend declaration and remove the private and public declarations. Keep it simple ...


7

Yes, this clever code solves your problem : you won't be able to nest commit_on_success or atomic under commit_on_success. You already know the caveats, but I'll repeat them anyway: If you do nest a block under a commit_on_success, your code will consistently fail: easy to fix as long as this code path is executed in your tests. It's always possible to ...


6

I thought about a comment, but too long, several things I can think of: This does not guarantee order, i.e. an item is entered into the next free slot, and if there is a fast producer, the order the consumer sees is not guaranteed. Is this intentional? There is no atomic combined detach/attach operation, this means that for example: Thread A: Attach starts, ...


6

The fact that you must be able to swap in a new set of maps at any time--even if only once every three months--requires a design change. Here are the requirements as I understand them: Reads block until all three maps have been set the first time. Reads receive a consistent set of maps. In other words, you cannot return the old secondary map after returning ...


6

When I looked first I was thinking you don't need it cause ConcurrentHashMap has atomic operations. But when I read your full ClientData I see that you create a map where the key is the same as your value. Next thinking of me : Isn't that a little overkill? Why don't you just put a Set in your AtomicReference? The Set isn't atomic so then you shall need ...


6

Even if in 1 line public static void blockHost(String hostName) { blockedHosts.get().put(hostName, hostName); } effectively consists of 2 operations, so if you want your methods to be atomic you still need synchronization. You could have an interleaving like: T1 invokes blockHost("123") and executes get() T2 invokes isHostBlocked("123"), does ...


6

Difference in default constructor std::atomic<T>'s default constructor is trivial - yours isn't. If your goal is to simply add copy semantics, I would maintain this same behavior: CopyableAtomic() = default; Memory orders I'm not sure about relaxed here. You probably want to ensure consistent orderings where you use this object, so I would change ...


5

There are a few things to go through, and then a suggestion. You say you are using a CountDownLatch in your description, but you are not. You expect there to be null values in the dataset of hosts, that 'smells'. You take a 'defensive copy' of the hostnames so that you can loop over them, even though, in the normal case, you only need the first host in the ...


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


4

For adding and removing single host names, I would use a simple ConcurrentHashMap without the AtomicReference. Initialize it with an empty map and drop the additional latch. Update: I really doubt you need to replace the list of hosts all at once, but here's the combined form anyway: private static final AtomicReference<ConcurrentHashMap<String, ...


4

AtomicReferences are a great construct when you have just one item that needs to be kept in a sane state ina multithreaded environment. Your code is trying to juggle three, and ensure they all have a sane state at the right times, and, for that, you need something bigger than the AtomicReference. You have elected to use the CountDownLatch. This, frankly, ...


4

To answer the issue you raised in a comment: I have this code running for a while in production and the problem I am seeing is as soon as hostA goes down, I start seeing lot of java.util.concurrent.TimeoutException on my client as you can see I am using Callable so which means I am doing future.get somewhere and it throws TimeoutException as soon as any ...


4

Your code is thread-safe as it is. [But not really... see the comments below.] Be careful with method and variable names. TempScheduler does not really have anything temporary. callServiceURL does a lot more than calling a URL. blockHost(hostname) would be better as addToBlockedHosts(hostname). call is non-descriptive. ClientData only contains ...


4

As I've stated in my previous answer, the first thing you must decide is "What level of atomicity does this application require?" I don't mean simple thread-safety, which the above code provides by not throwing exceptions or behaving in bizarre ways. Here are two examples where atomicity is violated: The first case, as Totò points out, can happen when ...


4

Pretty sure this is broken in multi-threaded code: do { // Point A. if (!old_front) return nullptr; new_front = old_front->next; } while(!front_.compare_exchange_weak(old_front, new_front)); What happens if: Thread A. Reaches point A Thread A is de-scheduled and is not currently running. Thread B. runs through ...


4

Negative permit count check In tryDecrementPermitCount(), I think this check: if ( oldPermitCount == 0 || newPermitCount < 0 ) return false; should just be: if ( newPermitCount < 0 ) return false; The original if statement confused me because both sides of the || are essentially checking the same thing. Also, the comment for the ...


4

Serious issues with this: Memory Management. Not using RAII to lock/unlock Memory Management Don't pass pointers it does not indicate owners. Always wrap pointers in a smart pointer. Have a look at std::unique_ptr. But for types like Data there is no need to use pointers. Simply use Data as the object type (not Data*). RAII Look up the concept of RAII. ...


3

Regarding the conditional return types in push() and pop(), it may be clearer to use a different type of infinite loop, such as a for(;;). This will better focus the attention to the loop contents. You could also use curly braces for the single-line statements. Some of these lines are quite long, so the executed code is at the very end. This should also ...


3

I can't find anything wrong with this. Just ... nitpicks. You name your variables in for each loops t. for (T t : supplier.tasks()) { taskQueue.add(t); } I think if you gave them a better name like task, it would improve readability. Your test code seems to be on emergency rationing for variable name characters. private ...


3

I just have some stylistic things for a start: Since your custom types are in PascalCase, your function names should instead be in camelCase or in snake_case. This isn't a requirement, but it makes it easier to distinguish between them. The naming convention used for the private members looks like a form of Hungarian Notation, which is quite disliked in ...


3

If the producers run faster than the consumer, then entries in the CircularRing will eventually be overwritten. This will also expose concurrent access to the LoggableEntity objects, which isn't thread-safe. When you are doing bulk-reads from the buffer, you can also bulk-increment the consumer index and potentially save CAS instructions. If this buffer ...


3

Konrad's answer has a big advantage: it is safe! In your code sample, if an operator raise an exception before you leave the critical section, this one will never be released. In Konrad's answer, not matter what, the critical section will be unlocked when you exit the scope, even if you don't catch an exception.


3

Order of pops is strange If I am reading your code correctly, your pop ordering will be different from the push ordering. Suppose we push 1 2 3 4 5 in that order. Your inbox will look like: 5 -> 4 -> 3 -> 2 -> 1 So far so good. But then when we pop for the first time, this is what happens: ret = 5 outbox = 1 -> 2 -> 3 -> 4 Then ...


3

idle As coded, idle is false when thread is in fact idle. Either rename it to working/running/whatever active or swap the true/false values. Control flow Unless I am missing something crucial, there is no need to goto neither in Worker nor in ThreadPoolAddTask. Similarly, found seems redundant: do { for (i = 0, i < MAX_WORKERS; i++) { if (....


3

Your implementation of Double Checked Locking is semantically correct; the acquire and release fences provide minimal necessary ordering. I am wondering why you are using standalone fences, which is almost never necessary.. It is more common to use memory ordering directly on atomic operations: auto sp = std::atomic_load_explicit(&(it->second), std::...


3

Primarily I'm looking for confirmation that it actually is atomic. Yes, it appears atomic. Other issues Range and precision randomicDouble() needs more functional explanation about range and precision. It seems to attempt an about linear distribution between [a...b] (b included). A range of [a...b) (b excluded) is quite common. So detailing the ...


3

Looks simple enough. You've got some nice little idioms for dealing with C's version of "header-only libraries" here. I'm mostly a C++ programmer, so I'm not 100% sure — C still doesn't have a way to say "this function is defined inline in a header"? it's still got the two-level inline/extern inline system going on, which is why you have to use static if you ...


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