Node.js race conditions | Hacker News Reader

Node.js race conditions

nodejsdesignpatterns.com

81 pointstodsacerdoti50 comments

Mutex in Nodejs, ROTFL. This kind of bad programming leads to overuse of synchronization primitives. Nodejs is single threaded and doesnt need locks and mutexes. In any application. Properly writing code will make synchronization unnecessary. In the provided example, the saveBalance function should not overwrite the values of balance variable but rather increment its value (increment with negative too). That's it your problem is solved, one line of code to change.

Race conditions are in the nature of concurrent code. Not just parallel code.

Take go, use gomaxprocs and set it to 1. Spawn a bunch of goroutines that share memory. And easily you'll have race conditions if you're not careful.

I didnt say race conditions are impossible in Nodejs. Just highlighted that good program engineering is enough to avoid it and programmers should do that in the first place.

Sure but at the same time there is a reason synchronization primitives exist.

Readability of code > everything else.

Nodejs doesn't have synchronization primitives, just because you use Promise and assign it to variable "mutex" doesn't make it any more real

What do you think is the real thing? It's also a value in a memory cell.

Yup :)

https://github.com/golang/go/blob/master/src/sync/mutex.go

You can easily see what Go does here. It's not magic.

It would be better if I had the time today to refute this in detail, because it's important that others reading this thread know this assertion is false, but...it's false.

Agreed Over the past 10 years, I've done async code to coordinate lots of different concurrent tasks in JS, never once needed a the use of a mutex.

That assumes that the underlying implementation of saveBalance can be updated to push the atomic operation downstream.

In general you highlight the problem though; without the central datastore ensuring atomicity, this isn't really solving anything.

I.e., if this app had multiple instances, you STILL have a problem, even with this solution, since nothing guarantees that another -instance- hasn't also loaded the old data.

Exactly, the author at the end explains that his approach doesnt work when you're using cluster/pm2/worker because these are separate processess, that just makes me laugh again as the whole post is pointless.

The (somewhat contrived) example is trying to model what happens when the state change is done with I/O (hence by definition asynchronous). Your idea to make the state changes idempotent is a good one, but not typically possible.

Read the post title once again: "Node.js race conditions", why mention I/O when the post is not about it?

You're right that this example is better solved without mutexes, but it's good that the article is introducing the concept to NodeJS devs who may not be used to it. Heck, I used mutexes extensively for years in C code and even I didn't know they were an option.

That said, mutexes are also not always wrong. Compare-and-swap is good, ensuring atomicity is good, but sometimes a mutex is what you need, especially if you're fixing legacy code with concurrency issues.

> A race condition is a type of programming error

I don't think a race condition is inherently an error. If you can design your program to accommodate races, including even data races, then allowing them and not spending time on protecting against them can be a useful performance optimisation.

It's only a bug if the race causes you to get into a state that you don't want.

I seriously doubt people writing web software are planning for intentional races in their system designs the way an OS dev does to avoid synchronization overhead.

I don't await promises if I don't need to. Don't think that's a super unusual pattern.

I feel a bit ignorant, although you don't need to await on assignment, don't you need to await to "unwrap" it when needed?

Sometimes you dont need the result.

Right, if you don't need it, you don't need to unwrap it.

As the article is making a round-about way of pointing out, you might need to "block" (unwrap) due to a side-effect in order to make the sequencing correct for downstream logic that touches the same thing.

Shouldn't be necessary if things are coded appropriately in the first place, and maybe better to just refactor, but...

It’s just a function that returns a promise object. The await keyword triggers the wait and unwrap functionality, but in cases where you don’t care you can just call it as a regular function

You should however, catch it.

If you're doing nothing with a promise (awaiting, or catching), then you have bad code.

You can read more about that here: https://palantir.github.io/tslint/rules/no-floating-promises...

That guide says that you should do this because it can cause race conditions:

> This can cause unexpected and/or non-deterministic behavior depending on external timing factors.

But, as this whole thread is about, sometimes you don't mind race conditions. So it can ok to acknowledge but ignore this advice if you are aware of how this race conditions matters to your codebase.

Learn the rules, but know when to break them.

Callbacks (including those passed to Promise then methods) are a dangerous primitive. I would recommend using awaitable streams like those used in https://socketcluster.io/ - Or https://github.com/SocketCluster/writable-consumable-stream for general purpose.

The philosophy of SC and WritableConsumableStream is that the main trunk logic of your system/module should declared as a single serial stream and any parallel logic should explicitly branch off from it.

With traditional event listeners, there is no such thing as a 'main trunk' of the logic... The entry points (event listeners) are scattered all over the place and this can cause a lot of possible state permutations when business logic is executed inside those listeners (since you can't control the timing of when those event handlers are called).

This kind of programming is a good alternative to functional programming as it offers a safe way to asynchronously mutate state.

Nice, are you using it in production? First time I hear about SocketCluster

Nice reminder that different kinds of race conditions exist, even without shared mutable data!

One thing I wish the article mentioned: In the given example, another solution would have been to make the balance change safe. E.g. `addBalance()` or `updateBalanceIfEqual(old, new)`. Requiring a mutex around each call site that needs to be used correctly (and keeps being used correctly over time) seems like a pretty fragile way out.

In a real world use case you are going to have multiple instances of your application running at the same time anyways, so the mutex solution is pointless. Use a DB transaction or something.

DB transaction will not save you in this case.

As the article hinted you have to either update the balance in one query or use a lock, like an optimistic lock.

Optimistic lock could have also been used instead of the mutex.

If you have multiple instances, a lock of any sort won't work; you have to update the query.

If you update the query, a lock is unnecessary.

I think it's fair to assume the parent was inferring that if you were using a DB transaction, you'd rewrite the logic to use it, rather than keeping the read and the write separate from each other, and therefore make a transaction pointless.

No, locks work. Lock is needed if you need to do some sort of complex computation before writing, like updating a JSON blob.

As both I and the parent said, "If you have multiple instances".

In which case, locks (like those mentioned in the article) don't work. Because instance A reads 0, instance B reads 0, instance A writes 50, instance B writes 50. Totally serialized on each instance, totally unpredictable in when they hit the DB.

Just like you have a lock serving as a synchronization point on an instance, in the case of multiple instances, you want to push the synchronization point down into a single instance at the data layer (which you often have when using a traditional RDBMS). But RDBMS' don't have locks, per se, for that synchronization; they have transactions that serve a similar purpose.

But, regardless of where that synchronization is, you have to ensure the update logic stays there. Having an interface that loads data, and saves data, at the application layer, ensures that a DB layer lock won't work, either. You have to change it to have the DB actually read and update the data to benefit from the synchronization the DB transaction provides you.

So, as I said before...a lock (at the application layer) is neither sufficient nor necessary. A DB transaction (with the implied update of the query necessary to make a transaction mean anything) is both.

Article talks about two different types locks, one is the mutex, which the article admits won’t work when using with multiple instances.

The other is briefly mentioned with a link to Wikipedia and is a optimistic lock that is to be used with your database.

I talked about two use cases for optimistic locking, one applied with a database, that with will work with any number of instances, because the synchronization check lives in the database and not in the instance.

My other case was to use an optimistic lock instead of the mutex example, that will still only work with single instance because the synchronization check is local to the instance. (Edit: wrong, it could actually work there too, if saveBalance() was changed)

Optimistic locking is quite simple to implement, just check that the value has not changed before writing, if it has you have to reread the value again.

Because you have updated your reply with more information I make a new reply instead. My old reply still stands about the what the article said and my use of optimistic locking.

Databases do support locks, usually called pessimistic locking (opposite of optimistic locking). In a RDBMS that is usually a table lock or a row lock.

Table locks are usually bad for performance and row locks better, however both are clumsy to work with, this is where the simplicity of optimistic locking comes in.

Where pessimistic locking is part of the RDBMS itself, optimistic locking is something that you implement yourself on top of the RDBMS.

Optimistic locks are lightweight (optimal case one read and one write) and easy to implement and understand.

In the example in the article I would have argued for optimistic locking on all cases and it would have worked with multiple instances.

Transactions and locks does not serve a similar purpose.

Mutexes in a single threaded event loop? Wut?

This problem requires a compare-and-swap primitive, not mutexes.

To have a race condition, you first have to have a shared state. And in my experience, for almost all applications where NodeJS is a good fit (usually a CPU-light application that is serving web requests, each of them independent of others, and waiting for a long time on IO, usually different database requests and other external services), shared state is already a bad pattern.

> To have a race condition, you first have to have a shared state.

I think you're possibly confusing 'race condition' with 'data race'. You need shared state for a data race, but not for a race condition.

For example if you send a request to two web services, then wait for responses from either, then that's a race condition on which result comes back first, because the non-deterministic timing can change program behaviour. And that's not even necessarily a problem or a bug.

You can easily get race conditions even in a system that tries to minimise shared state, such as Erlang, even though they protect you from data races.

Technically in your example the shared state is the network.

No the situation still holds if you send two requests on different network cards to entirely different networks that share nothing.

In that case the shared state is the observer.

I don't think that's really a meaningful or useful definition of shared state for these purposes.

It is! You should consider your brain as basically a database distant from the software, when you observe a race condition it's the same phenomenon as writing to a database (except you don't have a transactional system that gets activated by default).

shared state and message passing are dual to each other.

i feel like this post creates the problem, and then solves the wrong problem

Yep - in order for this solution to work, all of the calls need access to the same mutex; they all need to be in the same process (this is noted toward the end).

If you can do that, just share an "account" object. Calling account.balance+=50 doesn't require a mutex.

It's important to figure out ahead of time which features of your system can be executed in parallel and which features should be executed serially.

Chromium is rendering the code blocks as single-line, and they're impossible to read.

Is it just me? Firefox seems to load them just fine.

They look fine for me in Chromium.

One of the authors here. Could you please provide more detailss (system, browser type and version) and maybe a screenshot?

Do you have JS disabled?

Cool article! Fun programming assignment - implement a Mutex using promises.

Reply on news.ycombinator.com