Eslint: require-atomic-updates false positive

[medikoo]

Tell us about your environment

• ESLint Version: 6.0.1
• Node Version: 12.4.0
• npm Version: 6.9.0

What parser (default, Babel-ESLint, etc.) are you using?

Default

Please show your full configuration:

https://github.com/medikoo/eslint-config-medikoo/blob/master/index.js

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

const fn = async someObj => {
  let { foo } = someObj;
  await Promise.resolve();
  // Possible race condition: `someObj.bar` might be reassigned based on an outdated value of `someObj.bar`
  someObj.bar = "whatever";
};

What did you expect to happen?

No error reported

What actually happened? Please include the actual, raw output from ESLint.

Error reported:

Possible race condition: `someObj.bar` might be reassigned based on an outdated value of 

Are you willing to submit a pull request to fix this bug?

Not at this point

[mysticatea]

Thank you for your report.

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

[medikoo]

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)

[stinovlas]

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.

[jwalton]

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.

[j0h]

@mysticatea could the following POC also be related to the issue?

function dataGetter() {
  return {};
}

function anotherDataGetter() {
  return {};
}

async function main() {
  let get_data = await dataGetter();

  const get_another_data = await anotherDataGetter(get_data.id);

  (() => {
    get_data[get_another_data.foo] = 1;
  });

  // Error Here: Possible race condition: `get_data` might be reassigned based on an outdated value of `get_data`.
  get_data = {
    ...get_data,
    ...get_another_data,
  };
}

main();

Demo Link

[ololoepepe]

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?

[Lonniebiz]

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

[ololoepepe]

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

[ronkorving]

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.

[j0h]

@ololoepepe

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.

I think that might be a non-issue, I don't think you should be setting up your eslint to run through entire node_modules. This would be outside your context/source code and it would be up to the original maintainer of that said library to fix those issues (or yourself without an appropriate PR).

[viktor-ku]

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.

[Lonniebiz]

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.

[ronkorving]

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

[ralfstx]

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';
});

[bcoe]

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

[togakangaroo]

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 }; 
    }

[dzek69]

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.

[lvkins]

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
}

[Morgul]

Just thought I'd say I was bit by this too.

Every project where I use express or knex and async/await triggered this. In none of the cases am I actually useing an outdated value, rather, I'm overriding a default value based on the return of an await.

[brfrth]

This is also triggered when you simply want to reset a variable.

let myClass = new MyClass('hi');

while(someCondition) {
  const result = await myClass.doAThing(); // instance changed and "used-up"
  if(result == 'hi') console.log(result);
}

myClass = new MyClass('hi');

The example doesn't make sense because the real-world thing involves event triggers and a quasi-loop. I simply want to overwrite the "used-up" object with a new one for the next invocation, but for some reason this is a race condition. The new assignment doesn't involve anything about the old assignment...

[frederikhors]

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?

[drahnieR]

Also, i'll give one of the cases i found here.
doSomething() just read from elem. but if the async call doesn't involve elem at all, lint error is gone.

async function demo(someArray) {
  return Promise.all(someArray.map(async elem => {
    const result = await doSomething(elem); 
    if (result) {
      elem.f1 = result.f1;
    }
    return elem
  }))
}

Demo

[stinovlas]

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?

[jyrkive]

I believe that 3) at least would make sense. The rule in its current form flags way too much completely fine code.

[medikoo]

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)

[mysticatea]

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

[kaicataldo]

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.

[kaicataldo]

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.

[ronkorving]

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

[Jessidhia]

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.

[ololoepepe]

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

[mikecbrant]

+1 (replying so I can watch issue)

[jwalton]

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

[platinumazure]

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. :smile:

[Morgul]

@platinumazure Thank you for the update, and thank you to the TSC for making these changes!

[ronkorving]

@platinumazure Thanks for the progress, that's great. May I ask why reverting the change to the require-atomic-updates rule was not under consideration? After that, the next step would be to fix it, and only then push that change. It's nice that eslint:recommended is "fixed", but it's still a broken rule.

[platinumazure]

May I ask why reverting the change to the require-atomic-updates rule was not under consideration?

It just wasn't on the meeting agenda, that's all. No ulterior motive here, as far as I know. Maybe @mysticatea can speak to what benefits the most recent change to the rule brought and can offer thoughts on whether reverting that change could make sense.

[ericmorand]

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?

[ronkorving]

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

[dzek69]

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 👍

[eslint[bot]]

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.

[gwardwell]

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.

[jwalton]

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 [e-mail] 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
.

[gwardwell]

@jwalton fair enough. I think I’ll take the route of removing from recommended when I can make time for a PR. I’m going to assume it works for some people, so it should remain.

[kaicataldo]

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.

[tranvansang]

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.

[JoshCoady]

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.

[andiby]

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.

[exos]

Add one (actually working) code snip:

    TagModel.getTags = async function(...tags) {
        debug(`Getting tags objects`, tags);

        const stags = tags.map((tag) => {
            return tag.trim().toLowerCase();
        });

        let existing = await TagModel.findAll({
            where: {
                tag: stags,
            },
        });

        debug(`There are ${existing.length} existins tags`);
        const news = stags.filter((tag) => {
            return !existing.find((x) => x.tag.toLowerCase() === tag);
        });

        if (news.length) {
            debug(`There are ${news.length} new tags, creating...`);
            await TagModel.bulkCreate(news);
            const ntagso = await TagModel.findAll({
                where: {
                    tag: news.map((x) => x.tag),
                },
            });
            existing = [...existing, ...ntagso];
        }

        debug(`Final`, existing.map((x) => x.get({plain: true})));

        return existing;
    };

[cburkins]

Agree, this rule seems to be a bit overly-aggressive to provide good value, disabling as well.

[eslint[bot]]

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.

[glen-84]

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

[kaicataldo]

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.

[denis-sokolov]

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"]
  }]
}

[ololoepepe]

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