search

algolia / firestore-algolia-search

Apache License 2.0
112 stars 35 forks source link

Race condition #32

Closed oodavid closed 3 years ago

oodavid commented 3 years ago

Context: We have our own Algolia sync cloud function.

Reading through your sources I can see you will have a race condition issue due to hot and cold-start functions. It's not the easiest to detect, but can cause havoc in production.

I'm not at the office till tomorrow, but I will record a demonstration of the issue and simple solution.

oodavid commented 3 years ago

As promised:

https://www.loom.com/share/f9ba58288092453c857c72a64d4f9612

The mad diagram I made:

Firestore       [RevA]            [RevB]   [RevC]
                |                 |        |
syncAloia #1    [Initialisation][RevA]·····[RevC]··············[GC]
syncAloia #2                      [Initialisation][RevB]··············[GC]
                                     |          |      |
Algolia                              [RevA]     [RevC] [RevB]
smomin commented 3 years ago

Hey oodavid, thanks for creating this issue. I just watched your video. Pretty simple change to reduce the race condition. I will go ahead and make this change.

smomin commented 3 years ago

Hey @oodavid , since Algolia provides a way to version data, I am opting to use this approach instead of doing the additional read of the data. Take a look at the PR and let me know what you think.

https://www.algolia.com/doc/guides/sending-and-managing-data/send-and-update-your-data/in-depth/handling-concurrency-with-versioning/

https://www.algolia.com/doc/api-reference/api-methods/partial-update-objects/#update-only-if-the-value-is-greater-than-a-numeric-attribute

oodavid commented 3 years ago

That's a great approach. I hadn't realised this was available in Algolia!

I won't have time to look at this till Monday (possibly this weekend). A quick eyeball of the PR looks solid.

We'll update our function to use this approach too