4 ms·

> I wrote some code to get a message out of a hash table. The message was going to be put there by another thread. There was a small chance of a race where it w

> I wrote some code to get a message out of a hash table. The message was going to be put there by another thread. There was a small chance of a race where it wouldn’t be there yet when I initially looked for it. The code looked something like this: while ((message = map.get(key)) == null && System.currentTimeMillis() < timeoutTime) { wait(1000); } > The wait() call blocks the thread, waiting for the notifyAll() from the thread that puts the message into the map. The 1000 means one second. The timeout was going to be on the order of five seconds. > The above code is simple and correct … it looks neither simple nor correct. If this is multithreaded code, "small chance of a race" is a smell, and "notifyAll" is probably a smell. The multithreading primitive that I'd want to see here is a condition, so named because it permits a thread to wait for a particular condition. A condition wakes the "notified" thread with a mutex already locked, so that the condition ("is there a message in the table?") can be checked without interference from other threads. If there is a message, you can remove it, process it, etc. without races, and if there isn't, you can atomically release the lock and go back to sleep (which is again a thing the condition provides; it is not two separate calls).

deleted 2y ago
[deleted]
- deleted 2y ago
  [deleted]
ryandrake 2y ago
The other surprising thing is that two code reviewers also evidently missed the opportunity to re-write it properly using thread synchronization primitives. Rather they argued about what to pass to wait(), except you shouldn't be using wait() or sleep() here at all!
- pockmarked19 2y ago
  At least in 2025 they could slap it into Claude and produce a better review without engaging in…shudders *thought*.
  - ryandrake 2y ago
    Well the article was in 2009. Hopefully someone's taken them aside and explained synchronization objects to them by now.
pockmarked19 2y ago
The first snippet of code makes me nauseous and the second makes me murderous, so the former is obviously better. That being said, these people should all be miles away from mission critical code.
- ryandrake 2y ago
  They should be miles away from production code, mission critical or otherwise. Unless they just love eating hard-to-reproduce bugs for breakfast. Do they teach mutexes and semaphores in university anymore?
  - deleted 2y ago
    [deleted]
merb 2y ago
That what was in my mind, the second I’ve seen the code. I would‘ be pushed so hard to make the while go away. In high level code, it’s simple wrong. Since his code is Java it looks even more suspicious because of the hashmap. It’s basically a ‚work around’ bugfix.
sam_goody 2y ago
So, how would you write that line?
- Jtsummers 2y ago
  Throw in a mutex and a condition variable. Many languages and systems (including C++) with locks allow you to set a timeout (either a specific time to wake or a delay and then wake). Use a notify in the write thread to wake the reader. https://en.cppreference.com/w/cpp/thread/condition_variable https://en.cppreference.com/w/cpp/thread/condition_variable - C++11 so not available in this form to the blog writer, but an equivalent was. EDIT: I had C++ on the brain, the code is in Java and they're already using notifyAll. The wait should just be to the timeout limit and not shorter. If the data is ready in under a second, the notifyAll will wake the thread and it can keep running and there's no risk of waiting an extra second (approx) past the actual deadline as the current code would do.
- IshKebab 2y ago
  Honestly the fact that a thread is even waiting for a map value to be filled in is a bit of a red flag. 99% of multithreaded code should be using channels or parallel for/map. If you have to have a thread wait for a value from another then use a condition variable like he said. https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/locks/Condition.html https://docs.oracle.com/javase/8/docs/api/java/util/concurre...
- deleted 2y ago
  [deleted]
- mystified5016 2y ago
  99.999% of the time if you've hardcoded a delay, you should have done anything else. The correct tool here is cross-thread synchronization. You can do this in many ways, but for a simple situation like this, I'd use a mutex or a semaphore or something. In my opinion as an embedded programmer, the only time it's appropriate to hardcode a delay this way is if you're actually waiting on a physical process to happen. For instance, when I bring up a power rail in my widget, I have a delay of a few hundred milliseconds for capacitors to charge, power to stabilize, and chips to boot. Waiting five seconds for another thread to push data is not appropriate. We have entire ecosystems of tools to communicate between threads. Sleeping a thread is not one of those, and you typically only see that if the author doesn't understand the tools properly. It's a very beginner mistake.
dkarl 2y ago
The code the author gave is Java, so the hash map is most likely a ConcurrentHashMap, and it's safe to access and modify concurrently. The author's code is simplistic and inefficient, but it won't deadlock, it won't corrupt the hash map, it won't go more than a second past the timeout, and it won't fail to retrieve the data if the other thread sets it in a reasonable amount of time. To me, the smell is not that the author is using the wrong multithreading primitives but that they're using multithreading primitives at all. The first step if they want a better solution (and if the context allows them to rewrite the code in question) is to pick one of the many pre-packaged concurrency abstractions that will be safer and more optimized than 99.99% of programmers could write for themselves. Unless they have an extremely unusual problem, which they almost certainly don't, they don't need to be messing with mutexes and conditions at all. Honestly, judging from the style of the concurrency, I would guess that they're working with 15+ year old legacy code, and it's very possible that there's a bizarre reason they can't fix it.
layer8 2y ago
The wait method can only be invoked when the object is locked (e.g. within a synchronized block). The method throws an IllegalMonitorStateException otherwise. This implies that the map access is protected by that lock. Entering the wait releases the lock, and leaving the wait reacquires the lock. Those lock operations also imply a happens-before ordering, meaning changes made to the map by another thread during the wait are guaranteed to become visible to the current thread. The code is perfectly correct, and straightforward if you’re familiar with Java concurrency basics. Wait and notify were designed to be used in that way to wait for a condition. This is (or was) a common Java synchronization idiom: https://wiki.sei.cmu.edu/confluence/display/java/THI03-J.+Always+invoke+wait%28%29+and+await%28%29+methods+inside+a+loop https://wiki.sei.cmu.edu/confluence/display/java/THI03-J.+Al... https://docs.oracle.com/javase/tutorial/essential/concurrency/guardmeth.html https://docs.oracle.com/javase/tutorial/essential/concurrenc... Regarding the “small chance of a race”, the code’s whole purpose is to remove that race by synchronizing properly, which it does. The author presumably didn’t show the surrounding lock because it is implied by the wait, and because they were assuming their audience to be familiar with the idiom.

deleted 2y ago

[deleted]

deleted 2y ago
[deleted]

ryandrake 2y ago

The other surprising thing is that two code reviewers also evidently missed the opportunity to re-write it properly using thread synchronization primitives. Rather they argued about what to pass to wait(), except you shouldn't be using wait() or sleep() here at all!

pockmarked19 2y ago
At least in 2025 they could slap it into Claude and produce a better review without engaging in…shudders *thought*.
- ryandrake 2y ago
  Well the article was in 2009. Hopefully someone's taken them aside and explained synchronization objects to them by now.

pockmarked19 2y ago

The first snippet of code makes me nauseous and the second makes me murderous, so the former is obviously better. That being said, these people should all be miles away from mission critical code.

ryandrake 2y ago
They should be miles away from production code, mission critical or otherwise. Unless they just love eating hard-to-reproduce bugs for breakfast. Do they teach mutexes and semaphores in university anymore?
- deleted 2y ago
  [deleted]

merb 2y ago

That what was in my mind, the second I’ve seen the code. I would‘ be pushed so hard to make the while go away. In high level code, it’s simple wrong. Since his code is Java it looks even more suspicious because of the hashmap. It’s basically a ‚work around’ bugfix.

sam_goody 2y ago

So, how would you write that line?

Jtsummers 2y ago
Throw in a mutex and a condition variable. Many languages and systems (including C++) with locks allow you to set a timeout (either a specific time to wake or a delay and then wake). Use a notify in the write thread to wake the reader. https://en.cppreference.com/w/cpp/thread/condition_variable https://en.cppreference.com/w/cpp/thread/condition_variable - C++11 so not available in this form to the blog writer, but an equivalent was. EDIT: I had C++ on the brain, the code is in Java and they're already using notifyAll. The wait should just be to the timeout limit and not shorter. If the data is ready in under a second, the notifyAll will wake the thread and it can keep running and there's no risk of waiting an extra second (approx) past the actual deadline as the current code would do.
IshKebab 2y ago
Honestly the fact that a thread is even waiting for a map value to be filled in is a bit of a red flag. 99% of multithreaded code should be using channels or parallel for/map. If you have to have a thread wait for a value from another then use a condition variable like he said. https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/locks/Condition.html https://docs.oracle.com/javase/8/docs/api/java/util/concurre...
deleted 2y ago
[deleted]
mystified5016 2y ago
99.999% of the time if you've hardcoded a delay, you should have done anything else. The correct tool here is cross-thread synchronization. You can do this in many ways, but for a simple situation like this, I'd use a mutex or a semaphore or something. In my opinion as an embedded programmer, the only time it's appropriate to hardcode a delay this way is if you're actually waiting on a physical process to happen. For instance, when I bring up a power rail in my widget, I have a delay of a few hundred milliseconds for capacitors to charge, power to stabilize, and chips to boot. Waiting five seconds for another thread to push data is not appropriate. We have entire ecosystems of tools to communicate between threads. Sleeping a thread is not one of those, and you typically only see that if the author doesn't understand the tools properly. It's a very beginner mistake.

dkarl 2y ago

The code the author gave is Java, so the hash map is most likely a ConcurrentHashMap, and it's safe to access and modify concurrently. The author's code is simplistic and inefficient, but it won't deadlock, it won't corrupt the hash map, it won't go more than a second past the timeout, and it won't fail to retrieve the data if the other thread sets it in a reasonable amount of time. To me, the smell is not that the author is using the wrong multithreading primitives but that they're using multithreading primitives at all. The first step if they want a better solution (and if the context allows them to rewrite the code in question) is to pick one of the many pre-packaged concurrency abstractions that will be safer and more optimized than 99.99% of programmers could write for themselves. Unless they have an extremely unusual problem, which they almost certainly don't, they don't need to be messing with mutexes and conditions at all. Honestly, judging from the style of the concurrency, I would guess that they're working with 15+ year old legacy code, and it's very possible that there's a bizarre reason they can't fix it.

layer8 2y ago

The wait method can only be invoked when the object is locked (e.g. within a synchronized block). The method throws an IllegalMonitorStateException otherwise. This implies that the map access is protected by that lock. Entering the wait releases the lock, and leaving the wait reacquires the lock. Those lock operations also imply a happens-before ordering, meaning changes made to the map by another thread during the wait are guaranteed to become visible to the current thread. The code is perfectly correct, and straightforward if you’re familiar with Java concurrency basics. Wait and notify were designed to be used in that way to wait for a condition. This is (or was) a common Java synchronization idiom: https://wiki.sei.cmu.edu/confluence/display/java/THI03-J.+Always+invoke+wait%28%29+and+await%28%29+methods+inside+a+loop https://wiki.sei.cmu.edu/confluence/display/java/THI03-J.+Al... https://docs.oracle.com/javase/tutorial/essential/concurrency/guardmeth.html https://docs.oracle.com/javase/tutorial/essential/concurrenc... Regarding the “small chance of a race”, the code’s whole purpose is to remove that race by synchronizing properly, which it does. The author presumably didn’t show the surrounding lock because it is implied by the wait, and because they were assuming their audience to be familiar with the idiom.