Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Increment does not appear to be atomic #8772

Open
4 tasks done
theolundqvist opened this issue Oct 8, 2023 · 11 comments · May be fixed by #8789
Open
4 tasks done

Increment does not appear to be atomic #8772

theolundqvist opened this issue Oct 8, 2023 · 11 comments · May be fixed by #8789
Labels
type:bug Impaired feature or lacking behavior that is likely assumed

Comments

@theolundqvist
Copy link

theolundqvist commented Oct 8, 2023

New Issue Checklist

Issue Description

obj.increment does not appear to be atomic.

Steps to reproduce

I have the following code in the beforeSave hook of "childClass" and child has a pointer "parent" to "parentClass".

await obj.get("parent").increment("number_children").save(null, {
    useMasterKey: true,
});

Actual Outcome

Creating two child objects simultaneously results in "number_children" increasing from 0 to 1.
This does not happen with:

await obj1.save();
await obj2.save();

but only with

await Promise.all(obj1.save(), obj2.save());

Expected Outcome

I expected the count to increase from 0 to 2.

Environment

Server

  • Parse Server version: 6.2.2
  • Operating system: MacOS Ventura
  • Local or remote host (AWS, Azure, Google Cloud, Heroku, Digital Ocean, etc): local

Database

  • System (MongoDB or Postgres): MongoDB
  • Database version: 6.0.10
  • Local or remote host (MongoDB Atlas, mLab, AWS, Azure, Google Cloud, etc): MongoDB Atlas

Client

  • SDK (iOS, Android, JavaScript, PHP, Unity, etc): js
  • SDK version: 4.2.0

Logs

@parse-github-assistant
Copy link

parse-github-assistant bot commented Oct 8, 2023

Thanks for opening this issue!

  • 🚀 You can help us to fix this issue faster by opening a pull request with a failing test. See our Contribution Guide for how to make a pull request, or read our New Contributor's Guide if this is your first time contributing.

@mtrezza mtrezza added the type:bug Impaired feature or lacking behavior that is likely assumed label Oct 8, 2023
@mtrezza
Copy link
Member

mtrezza commented Oct 22, 2023

Could you add a test case with that example that you already posted?

@theolundqvist
Copy link
Author

I will have to examinate if I am doing anything special on my side. The following test works:

  fit('regression test for #8772 (increment should be atomic))', async () => {
    Parse.Object.disableSingleInstance();

    Parse.Cloud.beforeSave('Parent', (req) => {});
    Parse.Cloud.beforeSave('Child', async (req) => {
      await req.object.get("parent").increment("num_child").save(null, {useMasterKey:true})
    });

    let parent = await new Parse.Object('Parent').save(null, {useMasterKey:true});
    
    const child = () => new Parse.Object('Child').set("parent",parent).save();

    // add synchronously
    await child()
    await child()
    parent = await parent.fetch();
    expect(parent.get('num_child')).toBe(2);

    // add asynchronously
    await Promise.all(Array.from({length: 40}, () => child()))
    parent = await parent.fetch();

    expect(parent.get('num_child')).toBe(42);

theolundqvist added a commit to theolundqvist/parse-server that referenced this issue Oct 24, 2023
@mtrezza
Copy link
Member

mtrezza commented Oct 24, 2023

I's be surprised if no atomic test already exists. If there really is none we can add your test in any case. Could you open a PR?

@theolundqvist
Copy link
Author

Actually I can't find any. I'll open a PR

@theolundqvist theolundqvist linked a pull request Oct 24, 2023 that will close this issue
1 task
@RahulLanjewar93
Copy link

RahulLanjewar93 commented Nov 23, 2023

Yea even for me the updates are not atomic

@theolundqvist
Copy link
Author

Yea even for me the updates are not atomic

Could you elaborate on this? The test passes. Can you provide an example?

@RahulLanjewar93
Copy link

Yea even for me the updates are not atomic

Could you elaborate on this? The test passes. Can you provide an example?

Yea i tried as well the test you added is passing.
I am using multiple instances of parse server and there are cases where I am updating the object from all the servers at sa,e time using increment op.
But it seems the increment isn't atomic when using multiple instances.

@theolundqvist
Copy link
Author

theolundqvist commented Nov 23, 2023

@RahulLanjewar93 Why could it be working in parse-server tests but not in our own code?

Did you run the test on your own infrastructure?

The increment operation should be sent directly to mongo so therefore it should always be atomic independent of how many instances you are using? I am only using one instance

@RahulLanjewar93
Copy link

Yea, increment ops in mongo are atomic.
I had tested it before in my own infrastructure where i was using multiple instances. Currently I am occupied with something else, i'll have a test again and update on the same.

@mtrezza
Copy link
Member

mtrezza commented Mar 25, 2024

But it seems the increment isn't atomic when using multiple instances.

Although I haven't looked, I doubt that there is anything instance specific in the request that is sent to the DB. Maybe it's a DB issue? Or maybe it's an infrastructure issue where requests are lost, time out, or similar?

Or maybe it is a more complex Parse JS SDK issue where multiple increments / decrements, and object re-fetches in between cause issues with caching? You could add a more complex test to the #8789 where you randomly increment / decrement / re-fetch an object in a loop and check whether the end result is still correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Impaired feature or lacking behavior that is likely assumed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants