eslint: require-atomic-updates false positive

I would say that if nothing else, error message is pretty misleading.

someObj.bar might be reassigned based on an outdated value of someObj.bar

That is simply not true, because someObj.bar = "whatever"; assigns string "whatever" which is not depending on someObj.bar in any way.

Also, it’s not following the rule definition which clearly states that following must be true:

A variable or property is reassigned to a new value which is based on its old value.

I think this is intentional behavior because that someObj object was read before await expression and written after await expression.

Ok, but that’s pretty aggressive assumption, as in many cases that can be an intentional well-thought construct

In place where it reported for me, it’s totally intentional and there’s no way to write it differently (aside of dropping async/await), due to that I needed to turn off this rule globally.

Still I’d love it to report for me cases as count += await getCount() which are error-prone obviously (and even if not, easy to write in a way they do not report)

Per the October 10 TSC Meeting, we intend to take the following actions:

1. Update our semver policy to allow relaxing eslint:recommended in a semver-minor release (currently all changes to eslint:recommended are semver-major).
2. Remove require-atomic-updates from eslint:recommended in the next minor release.

Of course, we still need to find a way to fix the rule, but hopefully this will reduce pain short-term. 😄

There is a workaround:

// Given:
const obj = {foo: 'bar'};

// Instead of:
obj.prop = 'whatever';

// Do:
Object.assign(obj, {prop: 'whatever'});

This way the rule is not triggered (as expected). But here comes another question: given current (incorrect) behavior, shouldn’t the rule be triggered on Object.assign as well?

Reopening since this is an accepted bug. Assigning to myself so this doesn’t get auto closed again. We still need to figure out how we want to move forward here.

It is my understanding that await prevents any further execution from happening, within that code block until the promise is resolved. someObj is an argument passed into that function by reference and someObj is accessible everywhere it is used within that code block originally posted.

Even if you awaited a promise that did 100 things to the same object that someObj references, once that promise-work is done there is nothing to stop this code block from also setting someObj.bar = "whatever";. Sure, the fulfilled promise could have made AnotherReferenceTosomeObj.bar = "something specific";, but that should get over-written by someObj.bar = "whatever";

To assume that the promise does this on a “might”, and invoke an eslint error, seems like an attempt to oversimplify async programming to me.

If eslint isn’t sophisticated enough to point out BOTH lines of code, that are “racing” to overwrite something (race condition), then it probably needs to keep its might-suspicions to itself (until it gains that sophistication). Don’t block my build on a “might”. It takes two to race. If I’ve got a race condition, show me both lines of code that are racing each other.

Here’s my example: https://github.com/eslint/eslint/issues/11967

Here is a perfectly valid koa snippet that is faulted by this rule:

router.post('/webhook', async (ctx) => {
  await processRequest(ctx.request.body);
  ctx.body = 'OK';
});

Unfortunately, it looks like there wasn’t enough interest from the team or community to implement this change. While we wish we’d be able to accommodate everyone’s requests, we do need to prioritize. We’ve found that issues failing to reach accepted status after 21 days tend to never be accepted, and as such, we close those issues. This doesn’t mean the idea isn’t interesting or useful, just that it’s not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

Here’s a pretty vanilla Express middleware that triggers this problem, as you might see in any app that does virtual hosting based on the URL, if you’re looking for a concrete example:

export function populateAccount() {
    return function(req, res, next) {
        Promise.resolve()
            .then(async () => {
                const host = req.headers['host'];
                req.account = await getAccount(host);
            })
            .then(() => next(), next);
    };
}
/*eslint require-atomic-updates: error */

Demo link.

Sorry, folks - this should not have been closed. The TSC made a determination to remove this rule from eslint:recommended, and it looks like we forgot to update labels. I’ll make a PR for this right now.

Unfortunately, it looks like there wasn’t enough interest from the team or community to implement this change. While we wish we’d be able to accommodate everyone’s requests, we do need to prioritize. We’ve found that accepted issues failing to be implemented after 90 days tend to never be implemented, and as such, we close those issues. This doesn’t mean the idea isn’t interesting or useful, just that it’s not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

@mysticatea May I ask what the plan is? Will the behavior of this rule be reverted, or otherwise? Current behavior is keeping people from upgrading to ESLint 6. It would be good to know what the official stance is. If it’s “won’t fix”, then we’ll just all turn off this rule and move on with our lives. If it’s “will fix in 6.0.2”, then we know we can wait for a bit, and it will resolve itself.

If the error message reports that value was reassigned basing on previous value, but I am just overwriting a property without using any external values - then something is definitely wrong.

This rule wasn’t triggering false positives for me before 6.x.

Auto-closing an accepted issue with over 100 votes. 😕

@mikecbrant If you just want to subscribe to an issue, at the very top of this page, on the right hand side, under “assignees”, “labels”, etc… there’s a “subscribe” button that will notify you of updates to the issue:

Instead of +1ing the issue, please give it a “thumbs up” at the top, so the rest of us don’t all get a notification that you want to watch this issue. 😃

I think first straightforward and important step, is to remove it from eslint:recommended group.

Such controversial rule, which doesn’t work with many typical cases, should not be recommended (IMO we can consider it a bug, that it landed there)

I do not think this should be considered as a bug, it is a true positive warning, IMHO.

With the code snippet posted in OP, two different calls of fn with a same someObj do not mean the last value stayed in someObj.bar is assigned by the latter call of fn. Which explains why this pattern produces a race condition.

I have written a blog post about promise base semaphore pattern. This pattern can help avoiding this race condition effectively.

It seems that there are enough examples of this rule not working as intended (or at least not working as described in docs). I’d say that the discussion went long enough for the maintainers to chip in and give us some comment or resolution.

I think there are several possible courses of action:

1. Fix the rule to behave as described in the docs.
2. Fix the docs to describe the actual rule behaviour.
3. Disable this rule by default.
4. Remove this rule altogether.

@mysticatea Would you be so kind and comment on this?

I would like to speak up in support of the rule. Mutation and async programming is a source of many errors in the codebases I had worked on and this rule has been very beneficial to bring them to light. I would also like to speak up in favor of keeping it in recommended ruleset, to help expose these difficult issues for less experienced contributors. The false positives are, of course, a major issue, as it leads to the rule being disabled or ignored.

Perhaps we can introduce more ways to explicitly mark up objects that we are allowed to mutate? This can already be done somewhat using renaming a variable in one’s scope, but it looks like a hack, since at the end of the day the object maintains identity:

app.get('/', async (ctx) => {
  const myCtx = ctx;
  await process(myCtx.request.body);
  myCtx.body = 'OK';
});

An explicit comment could indicate the author’s intent and judgement. At this stage it is almost identical to eslint-disable-next-line require-atomic-updates, but with a better communication of the reasons (and it works for the entire block):

app.get('/', async (ctx) => {
  // eslint-we-own: ctx
  await process(ctx.request.body);
  ctx.body = 'OK';
});

For common patterns like the Koa context, it would be convenient to whitelist the variables we are allowed to mutate by name in the configuration:

{
  "require-atomic-updates": ["error", {
    "skip-variables": ["ctx"]
  }]
}

TSC Summary

require-atomic-updates is currently on in eslint:recommended and appears to be flagging safe code (see examples in comments above).

TSC Question

Can we remove this rule from eslint:recommended while we decide how we want to move forward with the rule? This should be fine to do in a semver-minor release because it will result in fewer errors being reported.

Still, I agree with you. That suspicion should not be considered an error. A warning would be quite enough.

I’m not even sure you should even warn when side-effects are obvious and likely intentional.

@ericmorand IMHO that is the responsibility of ESLint. It is to discourage use of patterns that are known to cause bugs. Example: you can omit break in a switch statement’s case, and this is a language feature, but is usually not what you want it to do. Omitting it may imply a bug. While testing may expose this bug, that is irrelevant to ESLint.

Why? This philosophy of reporting possible bugs (based on patterns that are considered dangerous) goes way back in the history of linting (pre-JavaScript). ESLint did not start this. So perhaps your expectations of ESLint are not in line with its philosophy? That is fine of course, but then perhaps consider using a different tool.

Do note that I’m not writing this as a defense for how this rule currently operates (scroll up to see my earlier comments).

Just my 2 cents.

I just got a build stuck because of require-atomic-updates even though the code works perfectly. I had to disable this rule. It doesn’t make sense to me. This is not eslint responsibility to try to guess that something may not work as expected. This is the reponsibility of the test suite.

Here we have a rule that throw an error saying:

**Possible** race condition...

Possible. Eslint is not sure for a good reason: it can’t be sure because only a test suite can be sure that a code works as expected.

More generally, there seems to be a lot of rules that try to guess that a piece of code will work as expected. Why is eslint doing this at all? Why trying to guess things that are anyway checked for sure by the test suite?

+1 (replying so I can watch issue)

this rule is triggering in a variety of scenarios on our repos that come up frequently in samples and tests, e.g.,

const _log = console.log;
const value = await Promise.resolve(10);
console.log = _log;

I’m in agreement with @ronkorving that this would be much more valuable as a more specific rule along the lines of, don’t do this:

count += await somePromise

This fails for React’s refs too:

const ref = useRef(false);

async function action() {
    ref.current = false;

    await something;

    ref.current = true; // Possible race condition
}

@Lonniebiz what if the other line of code is located somewhere in a 3rd-party lib? Looking through entire node_modules to find that line does no sound as a good idea. Still, I agree with you. That suspicion should not be considered an error. A warning would be quite enough.

You can make an other rule to handle all possible race conditions, but this rule has the name: require-atomic-updates. Please read the documentation for this rule

• A variable or property is reassigned to a new value which is based on its old value.
• A yield or await expression interrupts the assignment after the old value is read, and before the new value is set.

@stinovlas already mentioned this:

someObj.bar = "whatever"; assigns string "whatever" which is not depending on someObj.bar in any way.

Because of the last code changes this rule throws 4 errors for the Examples of correct code from the documentation:

let result;
async function foo() {
  result = await somethingElse + result;

  let tmp = await somethingElse;
  result += tmp; // wrong error: "Possible race condition: `result` might be reassigned based on an outdated value of `result`."

  let localVariable = 0;
  localVariable += await somethingElse;
}

function* bar() {
  result += yield; // wrong error: "Possible race condition: `result` might be reassigned based on an outdated value of `result`."

  result = (yield somethingElse) + result; // wrong error: "Possible race condition: `result` might be reassigned based on an outdated value of `result`."

  result = doSomething(yield somethingElse, result); // wrong error: "Possible race condition: `result` might be reassigned based on an outdated value of `result`."
}

Demo link

So please revert the code that it works again according to the documentation.

My 2 cents: all “possible” rules should exist, because usually they aren’t too much about guessing but about preventing bad practices and popular errors.

If you can fix the reported error by making your code better (no “hacks”, just really better) - then the rule is good. If you need to silence the rule, because the code is all good and the only alternative is to write hacky code to fool the linter - then the rule is bad.

So currently I have nothing against other rules, it’s just require-atomic-updates that went crazy after the update.

And the bonus thing: You can always change your config and disable all the “possible” rules if they are annoying to you 👍

I agree that eslint:recommended should not contain that rule because of too aggressive.

This is a real issue (I’m hitting all the examples pasted above), and one that was not mentioned in the migration guide for ESLint 6.0.0.

I can appreciate theories on how these cases are at least on paper unsafe, but that kind of behavior should then at least exist behind a flag. Right now, we’re losing a valuable feature because the only way to deal with this situation is to turn the rule off.

@denis-sokolov There are too many situations already, when people are treated as idiots (“for their own safety”, of course). Do not create another one, please.

Right now, we’re losing a valuable feature because the only way to deal with this situation is to turn the rule off.

That’s not the only option. You could just switch it to a warning:

  rules: {
    'require-atomic-updates': 1,
  },

It does warn about a valid issue that wont always crop up, but when it does, it is the type of insidious bug that could take days to discover. There are quite a few comments in this thread that belie the commenter’s understanding of parallel programming.

I do agree that this should be a warning and not an error.

This is not eslint responsibility to try to guess that something may not work as expected. This is the reponsibility of the test suite… Why trying to guess things that are anyway checked for sure by the test suite?

In my experience, most testers generally do not write test code to find race conditions—most automated tests I’ve seen test promises with a procedural mindset. This type of issue is generally far from “for sure” tested by the test suite.

It is my understanding that await prevents any further execution from happening, within that code block until the promise is resolved. someObj is an argument passed into that function by reference and someObj is accessible everywhere it is used within that code block originally posted.

This is wrong. await prevents any further execution from happening, within that invocation of that code block until the promise is resolved. someObj is an argument passed into that function by reference and someObj is accessible everywhere it is used within that code block and also everywhere else that has a reference to it.

This means that while waiting for the promise to resolve something else can modify someObj.bar and that by then setting it later, you are overriding that modification and it is unclear if it is the intention of the code to ignore those other potential changes.

I have a doubt about this code and this rule.

This is not good for eslint:

let controllerLogic

export default class {
  async start () {

    if (!controllerLogic) {
      const logic = await import('./controller.js')
      controllerLogic = logic
    }

    controllerLogic.go()

  }
}

This one is good for eslint:

let controllerLogic

export default class {
  async start () {

    if (!controllerLogic) {
      await import('./controller.js').then(logic => {
        controllerLogic = logic
      })
    }

    controllerLogic.go()

  }
}

WHY?

They are both bad, but it’s a bug in the rule that it cannot detect the second one as a problem.

The assignment is safe if and only if it is the only assignment done to the value AND the expression evaluated is idempotent. await import() happens to be, but the lint rule doesn’t know that.

Why? Because the update is not atomic. It is possible that, between the await starting and it settling, the value of controllerLogic changed, which makes the premise of the branch (that controllerLogic is falsy) now invalid. You need to do another compare-and-swap operation instead of assuming the value it had before await is the same as it has after.

For example, while controllerLogic is undefined, .start() is called multiple times before emptying the stack, there will be multiple assignments to controllerLogic pending resolution of the await for the result of the import() call. We humans know that import() will always return the same value for the same module (presuming no hot-module-reloading related bugs); but if each expression yielded a different value, you would have three different values for controllerLogic and controllerLogic.go, and the order they would execute is unpredictable (well, it is FIFO in the case of a normal import() implementation, but this is information we have that the lint rule does not).

This response is perhaps overly pedantic, especially for a non-parallel language like (most uses of) javascript, but this is the kind of thing you need to keep in mind when writing concurrent code; this is why Atomics exist, this is why atomic machine language instructions exists, this is why stdatomic.h exists.

I think we should add an option to ignore assignments to properties. That is, to report only assignments to variables if this option is enabled.

Just to add to the pile, here is one more very-common way of writing middleware that would trigger this

    const createNote = async (context) => {
        const { req: { entityId, entityType, note } } = context;
        const id = await notesModel.create(note, entityId, entityType);
        context.res = { id }; 
    }

The old implementation did add value, so I wish we could have that again. You could move the aggressive behavior behind an option.

I have a doubt about this code and this rule.

This is not good for eslint:

let controllerLogic

export default class {
  async start () {

    if (!controllerLogic) {
      const logic = await import('./controller.js')
      controllerLogic = logic
    }

    controllerLogic.go()

  }
}

This one is good for eslint:

let controllerLogic

export default class {
  async start () {

    if (!controllerLogic) {
      await import('./controller.js').then(logic => {
        controllerLogic = logic
      })
    }

    controllerLogic.go()

  }
}

WHY?

You can always submit a PR to fix it, or a PR to remove it from recommended.

On Sun, Nov 24, 2019, 18:58 Greg Wardwell notifications@github.com wrote:

Since this issue is now closed, I guess what we’re left with is turning this rule off in order to stop the false positives. If you, the ESLint team, won’t fix the bugs, then you should consider turning it off by default and making it opt-in or removing the rule altogether. 🤷‍♂️

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/eslint/eslint/issues/11899?email_source=notifications&email_token=AANQL6ZK4GM7DZBIAE2IQU3QVMIKRA5CNFSM4H3F2YFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFAYRXY#issuecomment-557943007, or unsubscribe https://github.com/notifications/unsubscribe-auth/AANQL67CN45JWWTEKX3UI53QVMIKRANCNFSM4H3F2YFA .

@Jessidhia it is still us humans who write code in 2019. We know better what we are doing (well, not all of us, but those who don’t hardly use linters), so the rule is too aggressive and should not be enabled by default in the form it is implemented now. It is kind of if some rule gave an error if we use (or do not use) some pattern, say, factory method. It is too opinion-based for an (not perfect) algorithm.

Agreed with @mysticatea. I’m not very familiar with this rule, but it sounds like we should consider doing the following:

1. Remove require-atomic-updates from eslint:recommended. We should be fine to release this in a semver-minor release because it will result in fewer errors being reported.
2. Update the rule so that it’s not so aggressive with what it flags and improve documentation around it’s behavior. If this is something we can’t enforce without false positives, I think we should reconsider the rule.

We have to disable this rule in our company due to many false positives. In paper rule is great and should prevent many bugs, but feels like it’s not quite ready yet.