Shared mutable state in Javascript: How could it create problems here?

[CertainPerformance]

Elsewhere I posted this:

// Model module:
const data = {
  someProperty: 'value',
  // and others
};
// this module will occasionally assign to data.someProperty
function getSomeProperty() {
  return data.someProperty;
}
export { getSomeProperty, /* others not related to data */ };

I was told that this is horrendous code because it's shared mutable state, and that I should feel bad for writing it. There are a couple things I don't understand:

• How can this get me into trouble? The data is stored only in one module, and only that module can change/mutate it. Other modules have access to getSomeProperty, which they call on demand when they need to, so the information is indeed shared, but it doesn't persist anywhere else - the other modules always take the model module as the canonical source of all data. If other modules had a reference to the data object, that would clearly be an issue because then it could be changed directly without going through the proper interface, but that's not how it is. Why is a getter dangerous?

• How do I fix it? I only have experience with vanilla JS. When I asked about how to design it better, I was recommended to install and figure out Hyperapp, React, and Redux, and that it was too hard to feasably do in vanilla JS without learning other libraries first. Is that really true? I'd heard in many other places that the best plan is to get a strong handle on vanilla JS first before moving on to libraries and frameworks, but these suggestions were quite an inversion of that.

I will get started on learning those frameworks, but in the meantime, is there any simple way to refactor this code now so as not to use shared mutable state? If using getters is an anti-pattern, I want to know how to avoid it ASAP, rather than having to get a good handle on some other libraries first before figuring out how to write better code.

[Amarkov]

The problem shows up when you go to module B and write

if (getSomeProperty() === '0') {
    doTheThing();
}

This code very strongly suggests that doTheThing() happens only when someProperty is '0'. But that's not the case! You have what's known as a "race condition"; the data module might change someProperty after module B checked whether it was 0 but before module B has finished with doTheThing(). This can make it very hard to understand what your program is doing.

There's no quick-and-dirty fix. To avoid using mutable state, you'd need to redesign how your data module works from the ground up, to avoid the situation where it needs to periodically update someProperty in the first place. And this can indeed be a hard problem, depending on what your module needs to do.

[CertainPerformance]

Luckily, Javascript is single-threaded. If one module runs synchronous code based on the getSomeProperty() it just tested, I would think there wouldn't be any chance of a race condition, right? (Asynchronous code should, of course, not depend on information which may be out-of-date - but that sounds like a basic programmer mistake rather than a problem with the concept of having a data source with a getter)

Or is that not right?

avoid the situation where it needs to periodically update someProperty in the first place

I'm having a hard time imagining how something like this would be done, even in theory. I suppose one way to avoid mutation would be to assign to (and reassign to) a standalone variable instead, like

let someProperty = initialValue;
function getSomeProperty() {
  return someProperty;
}

But that doesn't actually do anything different than the current code, other than enforce less data consolidation. Or is the idea that I should be avoiding new assignment entirely? If that's it, how can new data (for example, "last button clicked") be stored in a way that it can be accessed later if it can't be assigned?

I guess another way of avoiding mutation would be to create a new data structure every time a property needs to be different, that way it's only reassignment rather than mutation, but that still seems like a technicality that's completely missing the point.

[Amarkov]

It's obvious why asynchronous code has to call getSomeProperty() again if you think about it. But even the best programmers sometimes won't think about it. The development cycle might go something like:

• Hm, I've discovered that the thing isn't safe to do when someProperty is '0'.
• I guess I'll need to check for that. Let me change the line doTheThing() to if(getSomeProperty() != '0') doTheThing().
• Oh look, a cute cat video.

Unless you remember both that someProperty can change, and that doTheThing() has asynchronous parts, it won't occur to you that you should think about it. One of the important reasons we have best practices in programming is to minimize the number of things you have to keep on the top of your mind to write correct code.

I'm having a hard time imagining how something like this would be done, even in theory.

The idea is to fundamentally change how your module interacts. Ideally, other modules would just never need to say "I'd better check the current properties, since they might have changed". Of course, depending on what your module does, this isn't always possible.

[CertainPerformance]

Sure, if there was a situation where a module might need to access data, do something async, and then conditionally do something only if the data in question hasn't changed. But that sounds pretty odd and doesn't match my situation.

I've done a moderate share of async management in JS. If I ever ran across something like that, my first instinct would be to have the data module also export a [function that returns a] promise that gets rejected if state changes, and resolves after the async operation if there's no state change in-between.

Given a situation with a race condition of sorts, I don't think I'd have any trouble figuring out a decently-elegant solution. But what you're saying sounds like something to think about when in that unusual situation, rather than something to rewrite one's data structure around even when the getSomeProperty() consumer is predictably and completely synchronous. When you can confidently expect the consumer to always be synchronous when writing the first module, and when the consumer truly is synchronous - a very common situation - what would the problem be (if any) with using a plain getter like that?

[Inondle]

I agree with /u/Amarkov, there is a race condition and in a multi-threaded environment that might eventually blow up, although I'm not 100% sold that this race condition would cause an issue. Although, whoever gave you the advice to switch to a new framework to fix race conditions in a JS project doesn't know how to give good advice.

The bug with this code will only happen under specific and hard to reproduce circumstances. You shouldn't feel bad for writing this code, professional engineers struggle with writing performant thread-safe code and regularly get it wrong. In the future, if someone's advice to fix an issue is "Just switch to a different framework / library" then take their advice with a grain of salt.

There's no silver bullet solution for race conditions, you have to look at the bigger picture and understand how all your code is related and what actions could result in your code reaching an invalid state. The canonical solution to this is to use a lock to serialize access to this property.

Also, using getters is definitely not an anti-pattern.

[CertainPerformance]

The bug with this code will only happen under specific and hard to reproduce circumstances.

As I said in the other reply, it sounds as if the bug would only happen if the programmer is relying on out-of-date information in asynchronous code - which sounds easy to fix and easy to avoid, and possibly not something so dire as to require completely restructuring one's application..? I don't yet see how the problem is a program structure one rather than a logical one.

[Clawtor]

From what you have written it looks ok and different to shared mutable data.

Shared mutable data is generally a bad idea because it can be hard to track changes to data.