Skip to content

Commit 4c72955

Browse files
milkshakeukmilkshakeuk
authored
Merge pull request #251 from nHapiNET/ConcurrencyImprovements
Fix and improve concurrency issues
2 parents 27dfddf + 09b0e34 commit 4c72955

File tree

11 files changed

+686
-89
lines changed

11 files changed

+686
-89
lines changed

‎CHANGELOG.md‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,13 @@ All notable changes to this project will be documented in this file.
55
###
66
## [3.X.X.X] - TBC
77

8+
## [3.1.0] - TBC
9+
10+
### Enhancements
11+
- [#251](https://github.com/nHapiNET/nHapi/pull/251) Fix concurrency issues with `PipeParser`, `StructureDefinition`, `GenericMessage` and `Escape`.
12+
- [#250](https://github.com/nHapiNET/nHapi/pull/250) Add new options `DefaultObx2Type` and `InvalidObx2Type` to `ParserOptions`, ported from nhapi. (fixes [#63](https://github.com/nHapiNET/nHapi/issues/63))
13+
- [#240](https://github.com/nHapiNET/nHapi/pull/240) Add support for `NonGreedyMode` by introducing `ParserOptions` ported from hapi. (fixes [#65](https://github.com/nHapiNET/nHapi/issues/65) [#232](https://github.com/nHapiNET/nHapi/issues/232))
14+
815
## Previous Releases
916
###
1017

‎src/NHapi.Base/Model/GenericMessage.cs‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,16 @@
11
namespace NHapi.Base.Model
22
{
33
using System;
4-
using System.Collections.Generic;
4+
#if !NET35
5+
using System.Collections.Concurrent;
6+
#endif
57

68
using NHapi.Base;
79
using NHapi.Base.Log;
810
using NHapi.Base.Parser;
11+
#if NET35
12+
using NHapi.Base.Util;
13+
#endif
914

1015
/// <summary>
1116
/// A generic HL7 message, meant for parsing message with unrecognized structures
@@ -15,7 +20,11 @@ namespace NHapi.Base.Model
1520
public abstract class GenericMessage : AbstractMessage
1621
{
1722
// TODO: when we only target netstandard2.0 and above consider converting to ConcurrentDictionary
18-
private static readonly Dictionary<string, Type> GenericMessages = new Dictionary<string, Type>();
23+
#if NET35
24+
private static readonly SynchronizedCache<string, Type> GenericMessages = new SynchronizedCache<string, Type>();
25+
#else
26+
private static readonly ConcurrentDictionary<string, Type> GenericMessages = new ConcurrentDictionary<string, Type>();
27+
#endif
1928

2029
/// <summary>
2130
/// Creates a new instance of GenericMessage.
@@ -41,6 +50,10 @@ public GenericMessage(IModelClassFactory factory)
4150
"StyleCop.CSharp.NamingRules",
4251
"SA1300:Element should begin with upper-case letter",
4352
Justification = "As this is a public member, we will duplicate the method and mark this one as obsolete.")]
53+
[System.Diagnostics.CodeAnalysis.SuppressMessage(
54+
"Style",
55+
"IDE1006:Naming Styles",
56+
Justification = "As this is a public member, we will duplicate the method and mark this one as obsolete.")]
4457
public static Type getGenericMessageClass(string version)
4558
{
4659
return GetGenericMessageClass(version);
@@ -72,7 +85,7 @@ public static Type GetGenericMessageClass(string version)
7285

7386
if (c != null)
7487
{
75-
GenericMessages[version] = c;
88+
GenericMessages.TryAdd(version, c);
7689
}
7790
}
7891

‎src/NHapi.Base/Parser/Escape.cs‎

Lines changed: 26 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -28,12 +28,17 @@ this file under either the MPL or the GPL.
2828
namespace NHapi.Base.Parser
2929
{
3030
using System;
31-
using System.Collections.Generic;
31+
#if !NET35
32+
using System.Collections.Concurrent;
33+
#endif
3234
using System.Configuration;
3335
using System.Linq;
3436
using System.Text;
3537

3638
using NHapi.Base.Model.Configuration;
39+
#if NET35
40+
using NHapi.Base.Util;
41+
#endif
3742

3843
/// <summary>
3944
/// Handles "escaping" and "un-escaping" of text according to the HL7 escape sequence rules
@@ -48,13 +53,21 @@ public class Escape
4853
/// Is used to cache multiple <see cref="EncodingLookups"/> keyed on <see cref="EncodingCharacters"/>
4954
/// for quick re-use.
5055
/// </summary>
51-
private static readonly IDictionary<EncodingCharacters, EncodingLookups> VariousEncChars = new Dictionary<EncodingCharacters, EncodingLookups>();
56+
#if NET35
57+
private static readonly SynchronizedCache<EncodingCharacters, EncodingLookups> VariousEncChars = new SynchronizedCache<EncodingCharacters, EncodingLookups>();
58+
#else
59+
private static readonly ConcurrentDictionary<EncodingCharacters, EncodingLookups> VariousEncChars = new ConcurrentDictionary<EncodingCharacters, EncodingLookups>();
60+
#endif
5261

5362
[Obsolete("This method has been replaced by 'EscapeText'.")]
5463
[System.Diagnostics.CodeAnalysis.SuppressMessage(
5564
"StyleCop.CSharp.NamingRules",
5665
"SA1300:Element should begin with upper-case letter",
5766
Justification = "As this is a public member, we will duplicate the method and mark this one as obsolete.")]
67+
[System.Diagnostics.CodeAnalysis.SuppressMessage(
68+
"Style",
69+
"IDE1006:Naming Styles",
70+
Justification = "As this is a public member, we will duplicate the method and mark this one as obsolete.")]
5871
public static string escape(string text, EncodingCharacters encChars)
5972
{
6073
return EscapeText(text, encChars);
@@ -143,6 +156,10 @@ public static string EscapeText(string text, EncodingCharacters encodingCharacte
143156
"StyleCop.CSharp.NamingRules",
144157
"SA1300:Element should begin with upper-case letter",
145158
Justification = "As this is a public member, we will duplicate the method and mark this one as obsolete.")]
159+
[System.Diagnostics.CodeAnalysis.SuppressMessage(
160+
"Style",
161+
"IDE1006:Naming Styles",
162+
Justification = "As this is a public member, we will duplicate the method and mark this one as obsolete.")]
146163
public static string unescape(string text, EncodingCharacters encodingCharacters)
147164
{
148165
return UnescapeText(text, encodingCharacters);
@@ -252,7 +269,7 @@ private static EncodingLookups BuildEncodingLookups(EncodingCharacters encChars)
252269
if (!VariousEncChars.ContainsKey(encChars))
253270
{
254271
// this means we haven't got the sequences for these encoding characters yet - let's make them
255-
VariousEncChars[encChars] = new EncodingLookups(encChars);
272+
VariousEncChars.TryAdd(encChars, new EncodingLookups(encChars));
256273
}
257274

258275
return VariousEncChars[encChars];
@@ -276,7 +293,11 @@ public EncodingLookups(EncodingCharacters encoding)
276293

277294
public char[] SpecialCharacters { get; } = new char[8];
278295

279-
public Dictionary<char, string> EscapeSequences { get; } = new Dictionary<char, string>();
296+
#if NET35
297+
public SynchronizedCache<char, string> EscapeSequences { get; } = new SynchronizedCache<char, string>();
298+
#else
299+
public ConcurrentDictionary<char, string> EscapeSequences { get; } = new ConcurrentDictionary<char, string>();
300+
#endif
280301

281302
private LineFeedHexadecimal LineFeedHexadecimal { get; set; } = LineFeedHexadecimal.X0A;
282303

@@ -321,10 +342,7 @@ private void LoadHexadecimalConfiguration()
321342

322343
private void TryAddEscapeSequence(char key, string value)
323344
{
324-
if (!EscapeSequences.ContainsKey(key))
325-
{
326-
EscapeSequences[key] = value;
327-
}
345+
EscapeSequences.TryAdd(key, value);
328346
}
329347
}
330348
}

‎src/NHapi.Base/Parser/PipeParser.cs‎

Lines changed: 44 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,9 @@ namespace NHapi.Base.Parser
2828
{
2929
using System;
3030
using System.Collections;
31-
using System.Collections.Generic;
31+
#if !NET35
32+
using System.Collections.Concurrent;
33+
#endif
3234
using System.Linq;
3335
using System.Text;
3436

@@ -48,8 +50,15 @@ public class PipeParser : ParserBase
4850

4951
private static readonly IHapiLog Log;
5052

51-
private readonly Dictionary<Type, Dictionary<string, StructureDefinition>> structureDefinitions =
52-
new Dictionary<Type, Dictionary<string, StructureDefinition>>();
53+
private readonly object structureDefinitionLock = new object();
54+
55+
#if NET35
56+
private readonly SynchronizedCache<Type, SynchronizedCache<string, StructureDefinition>> structureDefinitions =
57+
new SynchronizedCache<Type, SynchronizedCache<string, StructureDefinition>>();
58+
#else
59+
private readonly ConcurrentDictionary<Type, ConcurrentDictionary<string, StructureDefinition>> structureDefinitions =
60+
new ConcurrentDictionary<Type, ConcurrentDictionary<string, StructureDefinition>>();
61+
#endif
5362

5463
static PipeParser()
5564
{
@@ -950,24 +959,42 @@ private IStructureDefinition GetStructureDefinition(IMessage theMessage)
950959
}
951960
}
952961

953-
Log.Info($"Instantiating msg of class {messageType.FullName}");
954-
var constructor = messageType.GetConstructor(new[] { typeof(IModelClassFactory) });
955-
var message = (IMessage)constructor.Invoke(new object[] { Factory });
962+
// Double-Checked Locking
963+
lock (structureDefinitionLock)
964+
{
965+
if (structureDefinitions.TryGetValue(messageType, out var definitions2))
966+
{
967+
if (definitions2.TryGetValue(theMessage.GetStructureName(), out retVal))
968+
{
969+
return retVal;
970+
}
971+
}
956972

957-
StructureDefinition previousLeaf = null;
958-
retVal = CreateStructureDefinition(message, ref previousLeaf);
973+
Log.Info($"Instantiating msg of class {messageType.FullName}");
974+
var constructor = messageType.GetConstructor(new[] { typeof(IModelClassFactory) });
975+
var message = (IMessage)constructor.Invoke(new object[] { Factory });
959976

960-
if (structureDefinitions.ContainsKey(messageType))
961-
{
962-
return retVal;
963-
}
977+
StructureDefinition previousLeaf = null;
978+
retVal = CreateStructureDefinition(message, ref previousLeaf);
964979

965-
var dictionary = new Dictionary<string, StructureDefinition>
966-
{
967-
{ theMessage.GetStructureName(), retVal },
968-
};
980+
if (structureDefinitions.ContainsKey(messageType))
981+
{
982+
return retVal;
983+
}
984+
#if NET35
985+
var dictionary = new SynchronizedCache<string, StructureDefinition>
986+
{
987+
{ theMessage.GetStructureName(), retVal },
988+
};
989+
#else
990+
var dictionary = new ConcurrentDictionary<string, StructureDefinition>
991+
{
992+
[theMessage.GetStructureName()] = retVal,
993+
};
994+
#endif
969995

970-
structureDefinitions[messageType] = dictionary;
996+
structureDefinitions.TryAdd(messageType, dictionary);
997+
}
971998

972999
return retVal;
9731000
}

‎src/NHapi.Base/Parser/StructureDefinition.cs‎

Lines changed: 78 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -7,17 +7,29 @@
77
/// <inheritdoc />
88
public class StructureDefinition : IStructureDefinition
99
{
10-
private HashSet<string> allChildrenNames;
10+
// TODO: Replace locks with Lazy{T} when support for net35 is dropped.
11+
// https://github.com/nHapiNET/nHapi/issues/253
12+
private readonly object allChildrenNamesLock = new object();
1113

12-
private HashSet<string> allFirstLeafNames;
14+
// TODO: Replace locks with Lazy{T} when support for net35 is dropped.
15+
// https://github.com/nHapiNET/nHapi/issues/253
16+
private readonly object allPossibleFollowingLeavesLock = new object();
17+
18+
// TODO: Replace locks with Lazy{T} when support for net35 is dropped.
19+
// https://github.com/nHapiNET/nHapi/issues/253
20+
private readonly object allPossibleFirstChildrenLock = new object();
21+
22+
private volatile HashSet<string> allChildrenNames;
23+
24+
private volatile HashSet<string> allFirstLeafNames;
1325

1426
private IStructureDefinition firstSibling;
1527

1628
private bool firstSiblingIsSet;
1729

1830
private bool? isFinalChildOfParent;
1931

20-
private HashSet<string> namesOfAllPossibleFollowingLeaves;
32+
private volatile HashSet<string> namesOfAllPossibleFollowingLeaves;
2133

2234
private IStructureDefinition nextSibling;
2335

@@ -110,24 +122,35 @@ public HashSet<string> GetNamesOfAllPossibleFollowingLeaves()
110122
return namesOfAllPossibleFollowingLeaves;
111123
}
112124

113-
namesOfAllPossibleFollowingLeaves = new HashSet<string>();
114-
115-
var nextLeaf = NextLeaf;
116-
if (nextLeaf != null)
125+
// Double-Checked Locking
126+
lock (allPossibleFollowingLeavesLock)
117127
{
118-
namesOfAllPossibleFollowingLeaves.Add(nextLeaf.Name);
119-
namesOfAllPossibleFollowingLeaves.UnionWith(nextLeaf.GetNamesOfAllPossibleFollowingLeaves());
120-
}
128+
if (namesOfAllPossibleFollowingLeaves != null)
129+
{
130+
return namesOfAllPossibleFollowingLeaves;
131+
}
121132

122-
var parent = Parent;
123-
while (parent != null)
124-
{
125-
if (parent.IsRepeating)
133+
var newNamesOfAllPossibleFollowingLeaves = new HashSet<string>();
134+
135+
var nextLeaf = NextLeaf;
136+
if (nextLeaf != null)
137+
{
138+
newNamesOfAllPossibleFollowingLeaves.Add(nextLeaf.Name);
139+
newNamesOfAllPossibleFollowingLeaves.UnionWith(nextLeaf.GetNamesOfAllPossibleFollowingLeaves());
140+
}
141+
142+
var parent = Parent;
143+
while (parent != null)
126144
{
127-
namesOfAllPossibleFollowingLeaves.UnionWith(parent.GetAllPossibleFirstChildren());
145+
if (parent.IsRepeating)
146+
{
147+
newNamesOfAllPossibleFollowingLeaves.UnionWith(parent.GetAllPossibleFirstChildren());
148+
}
149+
150+
parent = parent.Parent;
128151
}
129152

130-
parent = parent.Parent;
153+
namesOfAllPossibleFollowingLeaves = newNamesOfAllPossibleFollowingLeaves;
131154
}
132155

133156
return namesOfAllPossibleFollowingLeaves;
@@ -140,26 +163,37 @@ public HashSet<string> GetAllPossibleFirstChildren()
140163
return allFirstLeafNames;
141164
}
142165

143-
allFirstLeafNames = new HashSet<string>();
144-
145-
var hasChoice = false;
146-
foreach (var next in Children)
166+
// Double-Checked Locking
167+
lock (allPossibleFirstChildrenLock)
147168
{
148-
allFirstLeafNames.UnionWith(next.GetAllPossibleFirstChildren());
149-
150-
if (next.IsChoiceElement)
169+
if (allFirstLeafNames != null)
151170
{
152-
hasChoice = true;
153-
continue;
171+
return allFirstLeafNames;
154172
}
155173

156-
if (hasChoice || next.IsRequired)
174+
var newAllFirstLeafNames = new HashSet<string>();
175+
176+
var hasChoice = false;
177+
foreach (var next in Children)
157178
{
158-
break;
179+
newAllFirstLeafNames.UnionWith(next.GetAllPossibleFirstChildren());
180+
181+
if (next.IsChoiceElement)
182+
{
183+
hasChoice = true;
184+
continue;
185+
}
186+
187+
if (hasChoice || next.IsRequired)
188+
{
189+
break;
190+
}
159191
}
160-
}
161192

162-
allFirstLeafNames.Add(Name);
193+
newAllFirstLeafNames.Add(Name);
194+
195+
allFirstLeafNames = newAllFirstLeafNames;
196+
}
163197

164198
return allFirstLeafNames;
165199
}
@@ -171,11 +205,22 @@ public HashSet<string> GetAllChildNames()
171205
return allChildrenNames;
172206
}
173207

174-
allChildrenNames = new HashSet<string>();
175-
foreach (var next in Children)
208+
// Double-Checked Locking
209+
lock (allChildrenNamesLock)
176210
{
177-
allChildrenNames.Add(next.Name);
178-
allChildrenNames.UnionWith(next.GetAllChildNames());
211+
if (allChildrenNames != null)
212+
{
213+
return allChildrenNames;
214+
}
215+
216+
var newAllChildrenNames = new HashSet<string>();
217+
foreach (var next in Children)
218+
{
219+
newAllChildrenNames.Add(next.Name);
220+
newAllChildrenNames.UnionWith(next.GetAllChildNames());
221+
}
222+
223+
allChildrenNames = newAllChildrenNames;
179224
}
180225

181226
return allChildrenNames;

0 commit comments

Comments
 (0)