codecov-commenter
commented
1 year ago Codecov Report
Attention: 11 lines in your changes are missing coverage. Please review.
Comparison is base (
b652df4) 73.70% compared to head (d2157a7) 73.95%.:exclamation: Current head d2157a7 differs from pull request most recent head a0dcd00. Consider uploading reports for the commit a0dcd00 to get more accurate results
:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
```diff @@ 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 ``` | [Files](https://app.codecov.io/gh/moov-io/iso8583/pull/284?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io) | Coverage Δ | | |---|---|---| | [test/fuzz-reader/reader.go](https://app.codecov.io/gh/moov-io/iso8583/pull/284?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io#diff-dGVzdC9mdXp6LXJlYWRlci9yZWFkZXIuZ28=) | `55.55% <0.00%> (ø)` | | | [message.go](https://app.codecov.io/gh/moov-io/iso8583/pull/284?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io#diff-bWVzc2FnZS5nbw==) | `74.90% <91.83%> (+2.95%)` | :arrow_up: | | [field/composite.go](https://app.codecov.io/gh/moov-io/iso8583/pull/284?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io#diff-ZmllbGQvY29tcG9zaXRlLmdv) | `80.79% <85.36%> (-1.09%)` | :arrow_down: |:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
With this PR, we've enhanced the concurrency safety of the methods in
iso8583.Messageandfield.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 theMessageandCompositelevels. 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, andUnmarshalmethods, 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.