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.

Sign up for GitHub

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

Jump to bottom

Fix #115 Concurrency issue when adding documents. #116

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix #115 Concurrency issue when adding documents. #116

wants to merge 2 commits into from

Conversation

niemyjski
Copy link
Member

@niemyjski niemyjski commented Dec 23, 2024 •
edited
Loading

Fixes #115

System.InvalidOperationException: Operations that change non-concurrent collections must have exclusive access. A concurrent update was performed on this collection and corrupted its state. The collection's state is no longer correct.
   at System.Collections.Generic.Dictionary`2.TryInsert(TKey key, TValue value, InsertionBehavior behavior)
   at System.Collections.Generic.Dictionary`2.set_Item(TKey key, TValue value)
   at Foundatio.Repositories.Elasticsearch.Configuration.DailyIndex.EnsureDateIndexAsync(DateTime utcDate) in /_/src/Foundatio.Repositories.Elasticsearch/Configuration/DailyIndex.cs:line 150
   at Foundatio.Repositories.Elasticsearch.ElasticRepositoryBase`1.IndexDocumentsAsync(IReadOnlyCollection`1 documents, Boolean isCreateOperation, ICommandOptions options) in /_/src/Foundatio.Repositories.Elasticsearch/Repositories/ElasticRepositoryBase.cs:line 1252
   at Foundatio.Repositories.Elasticsearch.ElasticRepositoryBase`1.AddAsync(IEnumerable`1 documents, ICommandOptions options) in /_/src/Foundatio.Repositories.Elasticsearch/Repositories/ElasticRepositoryBase.cs:line 93
   at Foundatio.Repositories.Elasticsearch.ElasticRepositoryBase`1.AddAsync(T document, ICommandOptions options) in /_/src/Foundatio.Repositories.Elasticsearch/Repositories/ElasticRepositoryBase.cs:line 67

@niemyjski niemyjski added the bug label Dec 23, 2024
@niemyjski niemyjski requested a review from ejsmith December 23, 2024 19:10
@niemyjski niemyjski self-assigned this Dec 23, 2024
@niemyjski
Fix #115 Concurrency issue when adding documents.
8b4b117 
include date on lock

Tweaked logic
@niemyjski niemyjski force-pushed the bugfix/race-condition-index-creation branch from 174add4 to 8b4b117 Compare December 23, 2024 20:40
niemyjski
niemyjski commented Dec 24, 2024
View reviewed changes
src/Foundatio.Repositories.Elasticsearch/Configuration/DailyIndex.cs
protected async Task EnsureDateIndexAsync(DateTime utcDate)
{
utcDate = utcDate.Date;
if (_ensuredDates.ContainsKey(utcDate))
return;

await using var indexLock = await _ensureIndexLock.AcquireAsync($"Index:{GetVersionedIndex(utcDate)}", TimeSpan.FromMinutes(1)).AnyContext();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It kind of feels like we should be locking on the index name here, but this allows us to go wide. I'm wondering if we should also change this to a concurrent dictionary.

ejsmith
ejsmith reviewed Jan 1, 2025
View reviewed changes
src/Foundatio.Repositories.Elasticsearch/Configuration/DailyIndex.cs
protected async Task EnsureDateIndexAsync(DateTime utcDate)
{
utcDate = utcDate.Date;
if (_ensuredDates.ContainsKey(utcDate))
return;

await using var indexLock = await _ensureIndexLock.AcquireAsync($"Index:{GetVersionedIndex(utcDate)}", TimeSpan.FromMinutes(1)).AnyContext();
if (indexLock is null)
throw new Exception("Unable to acquire index lock");
Copy link
Contributor

@ejsmith ejsmith Jan 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should check to see if another process created the index we are looking for instead of just throwing here. Should just be able to check _ensuredDates.

ejsmith
ejsmith approved these changes Jan 1, 2025
View reviewed changes
Copy link
Contributor

@ejsmith ejsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this change. Should be good.

ejsmith
ejsmith requested changes Jan 1, 2025
View reviewed changes
Copy link
Contributor

@ejsmith ejsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, just thought about something. DailyIndex should be a singleton. Couldn't we just change the _ensuredDates to be a concurrent dictionary and not do a distributed lock?

@niemyjski
Copy link
Member Author

@ejsmith I thought about that, we probably could. That's making assumptions it is registered as a singleton and then there is alias management going on. Seems safer to do it like this.

@ejsmith
Copy link
Contributor

ejsmith commented Jan 2, 2025

There would actually be a single instance per daily index and we can count on that because that's how it has to be registered for a daily index to work. So I think just changing to a concurrent dictionary would work and is what we want to do.

@ejsmith
Copy link
Contributor

ejsmith commented Jan 2, 2025

There are just multiple threads trying to ensure an index for a specific day at the same time. Which is what the error you are seeing is from.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ejsmith ejsmith ejsmith requested changes

@twehner twehner Awaiting requested review from twehner

@randylsu randylsu Awaiting requested review from randylsu

Requested changes must be addressed to merge this pull request.

Assignees

@niemyjski niemyjski

Labels
bug
Projects
None yet
Milestone
No milestone
2 participants
@niemyjski @ejsmith