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

#2536 Fix race condition in bsonmapper #2538

Merged
merged 1 commit into from
Nov 14, 2024
Merged

#2536 Fix race condition in bsonmapper #2538

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Fix race condition in bsonmapper
  • Loading branch information
@JKamsker
JKamsker committed Sep 6, 2024
commit 4bdb4b700d0ed60e72656c84dc674a2c6004682e
50 changes: 36 additions & 14 deletions LiteDB/Client/Mapper/BsonMapper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,12 @@ public partial class BsonMapper
/// Mapping cache between Class/BsonDocument
/// </summary>
private readonly Dictionary<Type, EntityMapper> _entities = new Dictionary<Type, EntityMapper>();
JKamsker marked this conversation as resolved.
Show resolved Hide resolved


/// <summary>
/// Unfinished mapping cache between Class/BsonDocument
/// </summary>
private readonly Dictionary<Type, EntityMapper> _buildEntities = new Dictionary<Type, EntityMapper>();
Copy link
Member

Choose a reason for hiding this comment

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

how about using ConcurrentDictionary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't need concurrent access to this because it is only ever used in building the type mapping, which is always happening in the same thread.


/// <summary>
/// Map serializer/deserialize for custom types
/// </summary>
Expand Down Expand Up @@ -227,6 +232,8 @@ public BsonMapper UseLowerCaseDelimiter(char delimiter = '_')
#endregion

#region GetEntityMapper



/// <summary>
/// Get property mapper between typed .NET class and BsonDocument - Cache results
Expand All @@ -235,13 +242,30 @@ internal EntityMapper GetEntityMapper(Type type)
{
//TODO: needs check if Type if BsonDocument? Returns empty EntityMapper?

if (!_entities.TryGetValue(type, out EntityMapper mapper))
if (_entities.TryGetValue(type, out EntityMapper mapper))
{
return mapper;
}

lock (_entities)
Copy link
Member

Choose a reason for hiding this comment

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

You mentioned that is not needed to use ConcurrentDictionary, but we need a lock here in order to access and update its values (for both), are you sure isn't better to use that collection and avoid the lock here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the best case (which happens 99% of the time), we don`t lock. Only initially, when first creating the mapping, we need the lock.

In addidtion to that, this behaviour is the same as the original code before the bugfix that caused this bug

{
lock (_entities)
if (_entities.TryGetValue(type, out mapper))
{
if (!_entities.TryGetValue(type, out mapper))
return this.BuildAddEntityMapper(type);
return mapper;
}

if(_buildEntities.TryGetValue(type, out EntityMapper buildMapper))
{
return buildMapper;
}

var newMapper = new EntityMapper(type);
_buildEntities[type] = newMapper;
this.BuildEntityMapper(newMapper);
_entities[type] = newMapper;

_buildEntities.Remove(type);
return newMapper;
}

return mapper;
Expand All @@ -251,17 +275,17 @@ internal EntityMapper GetEntityMapper(Type type)
/// Use this method to override how your class can be, by default, mapped from entity to Bson document.
/// Returns an EntityMapper from each requested Type
/// </summary>
protected virtual EntityMapper BuildAddEntityMapper(Type type)
protected void BuildEntityMapper(EntityMapper mapper)
{
var mapper = new EntityMapper(type);
_entities[type] = mapper;//direct add into entities, to solove the DBRef [ GetEntityMapper > BuildAddEntityMapper > RegisterDbRef > RegisterDbRefItem > GetEntityMapper ] Loop call recursion,we stoped at here and GetEntityMapper's _entities.TryGetValue
// var mapper = new EntityMapper(type);
// _entities[type] = mapper;//direct add into entities, to solove the DBRef [ GetEntityMapper > BuildAddEntityMapper > RegisterDbRef > RegisterDbRefItem > GetEntityMapper ] Loop call recursion,we stoped at here and GetEntityMapper's _entities.TryGetValue

var idAttr = typeof(BsonIdAttribute);
var ignoreAttr = typeof(BsonIgnoreAttribute);
var fieldAttr = typeof(BsonFieldAttribute);
var dbrefAttr = typeof(BsonRefAttribute);

var members = this.GetTypeMembers(type);
var members = this.GetTypeMembers(mapper.ForType);
var id = this.GetIdMember(members);

foreach (var memberInfo in members)
Expand All @@ -288,8 +312,8 @@ protected virtual EntityMapper BuildAddEntityMapper(Type type)
}

// create getter/setter function
var getter = Reflection.CreateGenericGetter(type, memberInfo);
var setter = Reflection.CreateGenericSetter(type, memberInfo);
var getter = Reflection.CreateGenericGetter(mapper.ForType, memberInfo);
var setter = Reflection.CreateGenericSetter(mapper.ForType, memberInfo);

// check if property has [BsonId] to get with was setted AutoId = true
var autoId = (BsonIdAttribute)CustomAttributeExtensions.GetCustomAttributes(memberInfo, idAttr, true).FirstOrDefault();
Expand Down Expand Up @@ -324,7 +348,7 @@ protected virtual EntityMapper BuildAddEntityMapper(Type type)
}

// support callback to user modify member mapper
this.ResolveMember?.Invoke(type, memberInfo, member);
this.ResolveMember?.Invoke(mapper.ForType, memberInfo, member);

// test if has name and there is no duplicate field
// when member is not ignore
Expand All @@ -333,8 +357,6 @@ protected virtual EntityMapper BuildAddEntityMapper(Type type)
mapper.Members.Add(member);
}
}

return mapper;
}

/// <summary>
Expand Down