Thread-safe payment registration emulation practice

Question

I wrote this small service which emulates a payment registration system.

My main concern is thread-safety in general, and the caller shouldn't be able to register a payment while one is already in progress.

The service code:

using System.Collections.Concurrent;
using Microsoft.Extensions.Options;

namespace OpenPaymentApi.Services;

public class ThreadSafeCollectionsPaymentService : IPaymentService
{
    private readonly ConcurrentBag<RegisteredPayment> _payments = new();
    private readonly ConcurrentDictionary<string, object> _paymentRequestsInProgress = new();
    private readonly ServerSettingsOptions _serverSettings;

    public ThreadSafeCollectionsPaymentService(IOptions<ServerSettingsOptions> serverSettingsOption)
    {
        _serverSettings = serverSettingsOption.Value;
    }

    public async Task<Result<string>> RegisterPayment(string clientId, PaymentRequest paymentRequest)
    {
        var paymentInProgress = _paymentRequestsInProgress.ContainsKey(clientId);
        if (paymentInProgress)
        {
            return Result<string>.Error("Payment already in progress for the client");
        }

        _paymentRequestsInProgress[clientId] = new object();
        Thread.Sleep(TimeSpan.FromSeconds(_serverSettings.PaymentProcessingTimeInSeconds));

        var payment = new RegisteredPayment
        {
            Currency = paymentRequest.Currency,
            Creditor_Account = paymentRequest.Creditor_Account,
            Debtor_Account = paymentRequest.Debtor_Account,
            Transaction_Amount = paymentRequest.Instructed_Amount,
            Payment_ID = Guid.NewGuid().ToString(),
        };
        _payments.Add(payment);
        _paymentRequestsInProgress.Remove(clientId, out _);
        return Result<string>.Success(payment.Payment_ID);
    }

    public async Task<Result<RegisteredPayment[]>> GetPayments(string iban)
    {
        var result = _payments.Where(p => p.Creditor_Account == iban || p.Debtor_Account == iban).ToArray();
        return Result<RegisteredPayment[]>.Success(result);
    }
}

And I registered it as a singleton in program.cs like this:

builder.Services.AddSingleton<IPaymentService,ThreadSafeCollectionsPaymentService>();

and the service is being called from the controller like this:

        [HttpPost]
        [Route("/payments")]
        public async Task<IActionResult> PostPayment([FromHeader(Name = "Client-ID")] string clientId,
            [FromBody(EmptyBodyBehavior = EmptyBodyBehavior.Disallow)] PaymentRequest paymentRequest)
        {
            if (string.IsNullOrWhiteSpace(clientId))
                return BadRequest();

            var paymentResult = await _paymentService.RegisterPayment(clientId, paymentRequest);
            if (paymentResult.IsSuccess)
            {
                return Ok(paymentResult.Data);
            }

            return Conflict(paymentResult.ErrorMessage);
        }

Scorpion · Accepted Answer · 2025-01-17 06:25:59Z

No, this is not thread-safe.

    1️⃣ var paymentInProgress = _paymentRequestsInProgress.ContainsKey(clientId);
    if (paymentInProgress)
    {
        return Result<string>.Error("Payment already in progress for the client");
    }

    2️⃣ _paymentRequestsInProgress[clientId] = new object();

Assume two threads both call RegisterPayment at the same time for the same clientId. The intent is that they first check if a payment is in progress for this clientId (1️⃣) and return an error if so, or else register that an in-progress payment has begun (2️⃣).

The problem is that you have no control over whether a thread makes progress between when it performs the check at 1️⃣ and when it registers an in-progress payment at 2️⃣. Consider each row in this table as happening at a simultaneous slice of time.

Thread A	Thread B
Reaches 1️⃣	Reaches 1️⃣
No payment in progress, continue	No payment in progress, continue

In this case, because there is no locking or synchronization happening to prevent both threads from making progress for the same clientId, two payments will occur simultaneously.

Worse, the use of the indexer at 2️⃣ means that another payment could already be in-progress and get overwritten:

Thread A	Thread B
Reaches 1️⃣
No payment in progress, continue	Reaches 1️⃣
Reaches 2️⃣	No payment in progress, continue
Sets in-progress object for `clientId`	Reaches 2️⃣
Begins processing payment	Overwrites A's in-progress object for `clientId`
	Begins processing payment

Before the System.Collections.Concurrent types were introduced in .NET, this was a problem that would typically be solved with patterns like double-checked locking. However, the ConcurrentDictionary is made for this exact scenario. You should instead use the .GetOrAdd or .TryAdd methods on ConcurrentDictionary, which will atomically either add the in-progress marker or fail if one was already present.

Nice find. The meta point here is that so many threading problems can be found by just imagining two threads executing the code, each moving one line at a time. — Pierre Menard
– Pierre Menard, Commented Jan 18 at 0:49

Short Int · Accepted Answer · 2025-01-17 05:22:27Z

6

Replace Thread.Sleep with Task.Delay to avoid blocking the thread. Check out this article on Medium

answered Jan 17 at 5:22

Short Int

7622 silver badges14 bronze badges

Add a comment |

Atom Driver · Accepted Answer · 2025-01-17 06:26:04Z

If this were a real service, then you have to consider that payments can come in through multiple server nodes, not just the server running your code. To ensure that only one payment is in progress, you need to model the synchronization of that.

There are many ways to do that. One way, if you were using Azure Cosmos DB for transaction recording, for example, is to leverage optimistic concurrency on a document. Presuming there is a unique id for the customer, you could attempt to create a document with an id that's 1:1 for the customer id, and if that succeeds, you know that the server calling is the "winner". If it fails, you know that another transaction started (but not that it finished, if the process/server crashed in the middle of it). You can handle crashes either with a TTL on the document, or by storing an expiry in the document itself. If expired, you can delete it (passing the ETag, to avoid deleting someone ELSE'S retry).

Similar strategies can be used for different storage layers. If using SQL, you can attempt to insert a row into a table where the client id is a primary key. This will fail if one already exists, so you're guaranteed one winner again. The important thing is that there is a shared, authoritative place to record the transaction start.

Don't forget to clean it up when done, of course.

Also, as Charles noted, never use Thread.Sleep in server code. Always await Task.Delay instead. Even better, even for simulations, is to mock up the payment service's method call so that IT does the delay, not the calling code.

Some more nits:

Naming: It's not c#-y to use underscores in property names. The general practice is to use PascalCase. More importantly, if your team follows a different naming policy, use that and ignore this.
Input validation: The client id is not really validated, except for checking for whitespace/empty/null. How do you know it's valid and for a real customer?
Authentication: Related to the client id. Identity should always be strongly validated through a standard like JWT. Your client's identity should not just be passed as some HTTP header.

Regarding naming, I don't see any properties in OP's code. Are you talking about fields? — JAD
– JAD, Commented Jan 17 at 11:54

Stack Exchange Network

Thread-safe payment registration emulation practice

3 Answers 3

You must log in to answer this question.

Hot Network Questions

Thread-safe payment registration emulation practice

3 Answers 3

You must log in to answer this question.

Related

Hot Network Questions