-
Notifications
You must be signed in to change notification settings - Fork 112
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
Make the package safe for concurrent access #284
Conversation
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #284 +/- ##
==========================================
+ Coverage 73.70% 73.95% +0.24%
==========================================
Files 43 43
Lines 2282 2338 +56
==========================================
+ Hits 1682 1729 +47
- Misses 369 378 +9
Partials 231 231
☔ View full report in Codecov by Sentry. |
With this PR, we've enhanced the concurrency safety of the methods in
iso8583.Message
andfield.Composite
.Rationale
I encountered a data race when two goroutines executed
message.GetString(11)
. This happened because the GetString method did more than just returning a value — it also updated an internal map. This concurrent access led to the race condition.Assumptions on Concurrency:
field.String
,field.Numeric
) is not thread-safe. Concurrency safety is ensured at theMessage
andComposite
levels. If external concurrent access to individual fields is necessary, users should manage it with appropriate synchronization mechanisms, such as a mutex.Concerns
A potential solution to the data race I encountered would have been to simply eliminate the unnecessary map modification, thus making
GetXXX()
methods pure getters (which is also done in the PR). However, because the message is referenced via a pointer and has a combination of getters, setters,Marshal
, andUnmarshal
methods, it doesn't feel entirely safe to assume that access to these will always be sequential. So, enhancing concurrency safety seems like a guarded measure.