Every line is a potential bug (2009)

I think the analogy is a valuable one.

On an airplane, you might look for places where the airframe is stronger than it needs to be. If you can save material there, it'll be not just cheaper but lighter and more efficient. If the weight savings improves the flight characteristics of the aircraft, or if allows for the addition of safety features elsewhere, it might improve safety as well.

This is a really hard conversation to have in software, because when people think they can make something better with a lot of extra code, a lot of times they'll just do it, even if they know that other people think it's unnecessary. They put in the extra time on their own responsibility, thinking that the only downside is the initial work to write the code, and once it's done, it becomes an asset. It sucks to tell them that I'd rather not have it, that I'd rather have the easy version they could have done in an hour. I give in way more often than I'd like to.

  while ((message = map.get(key)) == null && System.currentTimeMillis() < timeoutTime) {
      wait(1000);
  }

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

My general rule when there's a potential for race condition bugs like this is you should multiply your paranoia for looking for bugs by at least 10, and very heavily consider alternatives that aren't subject to race conditions because it's impractical to get everyone who might review, extend or maintain the code to be sufficiently paranoid.

For race conditions, it feels like you really have to experience the wide potential for bugs yourself during development to understand how unpredictable, unintuitive, and catastrophic the unhappy code paths can be.

When I'm editing or writing code that needs to be really robust, I'm usually thinking about the state space and control flow diagram in my head. If there's too many paths that aren't likely to be explored during regular manual testing, test suite runs, and normal user usage, that's a sign to do something like push back on if the requirements need to be this complex, or if there's some code improvements we can make like refactoring or better abstractions. Otherwise there's likely to be subtle bugs hiding somewhere. Potential race conditions in particular usually explode the state space.

It turns out that you just need to make reasoned assessments about how the system[*] you're building on works; what you need to make it do today, what you're likely to need to make it do tomorrow; how your work impacts other parts of the system today and tomorrow; what resource pressures your system is facing; etc

As written with its language of absolutes, the article gives horrible advice. If you were to follow it at every opportunity, you would write software that becomes more and more terribly brittle, terribly inefficient, terribly illegible, terrible incapable over time.

Our craft is one of engineering. It takes years of study and practice to learn to do it well. You never stop learning to take more into consideration and never overcome having to make hard, arguable decisions in light of both known and unknown unknowns. It just doesn't reduce to adages like this and you start building a wall in front of your own professional growth when start assuming it does.

--

[*] where system is inclusive of your skills and fluency, your development tools, your deployment environment, your team and their skills and preferences, your stakeholders and their requirements, your users, your budget of time and capital and mental energy, etc

    while ((message = map.get(key)) == null) {
      long now = System.currentTimeMillis();
      if (now >= timeoutTime) {
        break;
      }
      wait(timeoutTime - now);
    }

    while ((message = map.get(key)) == null) {
      waitUntil(timeoutTime);
    }

You could have waitUntil return true or false to indicate whether the wait occurred or not, but then that loses the opportunity to convey whether the thread was awakened due to an operation on the map.

    do {
      message = map.get(key);
    } while (message == null && waitUntil(timeoutTime));

People will ask and expect every single part of your code to take in as much complexity as possible because "well, it makes sense". Take something as simple as a timeout value for a task.

- We need to add a timeout because list reasonable reasons to have timeouts

- Hey, so the timeout should only apply for external IO. Some tasks are just CPU intensive and that shouldn't cause the timeout to kick in. Like sure tasks usually take 10 minutes, but if it's taking more because of CPU, then we should let it.

- Hey, so we had an infinite loop bug in a task. So it was consuming a lot of CPU, but shouldn't have waited an hour to timeout. If there is no stdout, then its safe to assume it's stuck for other reasons.

- So this task runs ffmpeg, and there is no stdout, but it's actually running and should be fine. so check if ffmpeg process is running and handle it differently.

- Hey, so the timeout should be different for this one endpoint X. We know that it's slow, so we should increase the timeout only for that endpoint and not count it in the overall timeout.

- So that endpoint X has a fairly high timeout today. However, they had an outage yesterday and all our calls took hours to eventually timeout. We should have the timeout be adaptive depending on the outcome of previous attempts per endpoint.

- So the adaptive timeout logic kinda tripped over itself here. See, service Y is usually fast, so it's timeout is low. But last week they had an issue, and timeout was taking 20 minutes for all task. It looks like the adaptive-timeout-system took the timeout up to 20 minutes for it. But it shouldn't have. For service Y, it should have have only gone up to 2 minutes max. We need to add limits to the adaptive-timeout-system

- Can we determine these adaptive-timeout-system limits automatically? It's a pain in the ass to need to figure out what is the right timeout value for each service we call.

- So what if the value of the timeout came from an LLM. So we give the LLM the information about the current task, and what we know about the the previous calls, then the LLM suggests a value for the timeout.

Unfortunately not:

Using notifyAll and wait in Java is tricky to do correctly, you'd have a much easier chance using concurrency primitives that wrap these functions, like CountDownLatch[1] for one offs, or Condition[2].

System.currentTimeMillis is also not guaranteed to be monotonic, and System.nanoTime should be used instead. This would of course be obviated if a better concurrency primitive were to be used.

[1] https://docs.oracle.com/en/java/javase/21/docs/api/java.base...

[2] https://docs.oracle.com/en/java/javase/21/docs/api/java.base...

When what they really wanted here for bug free multithreading is some kind of promise or async await abstraction.

If you wait for the time between now and the timeout, you mandate that if the value is not in the hashmap immediately, the program will sleep for ~5 seconds no matter what. "the key was very likely to be there within the first second" is the entire rebuttal to their suggestion. You don't need anything else, why would you sleep for 5 seconds if you can only sleep for 1? Do you not care about runtime performance? Do you have no respect for the user who has to wait for the output?

Magic number (1000) unrelated to the timeout constant must have triggered strong emotions.

I think that the moral of the story is a bit different.

[1] https://en.wikipedia.org/wiki/Kolmogorov_complexity

Technically, the reviewer didn't introduce any new lines

https://github.com/pajecawav/ghloc-web#badges

That's the wrong attitude. Every line of code has the potential for multiple bugs. Don't be afraid to think big.

Every line is potentially /many/ bugs.