Fix idempotency key corruption in GpApiConnector under concurrent load - #101
Open
RadoslavSheytanovGP wants to merge 2 commits into
Open
Conversation
…rray JsonDoc.GetValue<T> could throw an uncaught InvalidCastException while mapping an Open Banking BankPaymentDetail response. When a field such as created_on came back as a JSON array it was stored as a List<string>; Convert.ChangeType then failed and the catch block cast the list straight to the target type, throwing a second time. That surfaced to callers as a 500 during ReportingService.BankPaymentDetail(...).Execute(). The catch now returns the value when it is already the requested type, otherwise collapses an array to its first convertible element, and falls back to default(T) instead of re-throwing. This covers every field mapped through GetValue<T>, not just created_on. Adds GpEcomOpenBankingMappingTest covering the arrayed and scalar cases.
GpApiConnector is registered as a per-config singleton, so a single instance serves every concurrent caller in a process. Several pieces of per-request state were written onto shared mutable fields: - DoTransactionWithIdempotencyKey wrote x-gp-idempotency onto the shared Headers dictionary and removed it in a finally block. Concurrent calls overwrote or removed one another's key, so idempotent requests were sent with the wrong key or none at all and the gateway could not deduplicate them, resulting in duplicate charges. - SendRequest enumerated the shared Headers dictionary while the AccessToken setter rewrote Authorization during a token refresh, which could throw "collection was modified". - ProtectSensitiveData / MaskedValueCollection used process-global masking state that was added to and disposed concurrently, corrupting the backing dictionary under load. Changes: - Thread the idempotency key (and any per-request header) through DoTransaction/SendRequest and apply it to the local request only; the shared Headers dictionary is never mutated per request. - Snapshot Headers under a lock that is also held by the Authorization writer before enumerating them. - Make ProtectSensitiveData and MaskedValueCollection thread-safe and hand out snapshot copies. Adds offline concurrency regression tests (local loopback listener) that fail on the previous code and pass with this change. Refs globalpayments#98
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
GpApiConnectoris registered as a per-config singleton (ConfiguredServices.GatewayConnector), so one instance serves every concurrent caller in a process. Per-request values were written onto shared mutable fields, which corrupts under concurrent load. This is the root cause behind #98 / AH-2819 (duplicate charges under concurrency).Races fixed
Idempotency key (the reported duplicate-charge bug).
DoTransactionWithIdempotencyKeysetHeaders["x-gp-idempotency"]and removed it in afinally. With concurrent calls on the shared instance, one thread overwrote or removed another thread's key, so idempotent requests went out with the wrong key or none — the gateway could not deduplicate, producing duplicate charges. The key is now threaded per-request and applied to the localHttpRequestMessageonly; the sharedHeadersdictionary is never mutated per request.Header enumeration vs. Authorization refresh.
SendRequestenumeratedHeaderswhile theAccessTokensetter rewroteAuthorizationduring a token refresh, which could throwInvalidOperationException: Collection was modified.Headersis now snapshotted under a lock that theAccessTokensetter also holds.Masking state.
ProtectSensitiveData/MaskedValueCollectionused process-global state added-to and disposed concurrently, corrupting the backing dictionary. Both are now thread-safe and hand out snapshot copies.Approach notes
null; the exact header-add order is preserved for all other gateways (Portico, NWS, GP-ECOM, etc.). No public API removed or changed.SemaphoreSlimworkaround some integrators applied as a stopgap.DoTransaction.Testing
New offline concurrency tests (
tests/GlobalPayments.Api.Tests/GpApi/GpApiConcurrencyTest.cs) run against a local loopback listener with a preset token (no sandbox required):ConcurrentCharges_EachRequestKeepsItsOwnIdempotencyKey— 500 concurrent charges; every request keeps its own key, none dropped or duplicated.ConcurrentChargesWithForcedReauth_KeepIdempotencyKeysAndDoNotThrow— forces concurrent re-authentication (Authorization rewrites) racing header snapshots.ConcurrentChargesWithProductionLogging_DoNotThrow— exercises the masking/logging path under load.All three fail on the current code (
Collection was modified/ non-concurrent-collection corruption) and pass with this change.Scope / follow-up
This PR resolves the reported idempotency corruption and the header-level races. One related item is intentionally out of scope:
Request.MaskedValuesis apublic staticfield used to pass masking data from the request builders to the logger. This change makes that path crash-safe, but under concurrent request-logging the masked-value set on one request's log line can still reflect another's. Fixing it properly means threading masking data per-request across the request builders and changing a public static member — a separate, API-affecting change best handled on its own. Happy to follow up.