Removing synchronization from an agent for a trading system

Is there any way to easily remove synchronization from this code, when there is a validation step before acquiring a lock?

I tried an AtomicReference> to hold the state. But that complicates the algorithm a lot because I have to use compareAndSet() in accept(), and then if that returns false, I have to redo the checks for isFinished(), isAboveDailyLimit(), aboveTradingLimit().

```
import wojtek.events.*;
import wojtek.tinytype.CounterParty;
import wojtek.tinytype.DailyLimit;
import wojtek.tinytype.TradeValue;
import wojtek.tinytype.TradingLimit;

import java.time.Clock;
import java.time.ZonedDateTime;

public class CheckTradeAgent {
private final CreditCheckAPI creditCheckAPI;
private final Clock clock;
private final CounterParty counterParty;
private final TradingLimit tradingLimit;
private final DailyLimit dailyLimit;
private final CheckTradeEventListener eventListener;

private final Object limitAndFinishLock = new Object();
private int utilisedDailyLimit;
private boolean finished;

public CheckTradeAgent(CreditCheckAPI creditCheckAPI, CheckTradeEventListener eventListener, Clock clock, CounterParty counterParty, TradingLimit tradingLimit, DailyLimit dailyLimit) {
this.creditCheckAPI = creditCheckAPI;
this.clock = clock;
this.counterParty = counterParty;
this.tradingLimit = tradingLimit;
this.dailyLimit = dailyLimit;
this.eventListener = eventListener;
}

public void handle(String counterParty, int tradeValue) {
try {
handle(new TradeSubmitted(new CounterParty(counterParty), new TradeValue(tradeValue), now()));
} catch (RuntimeException e) {
eventListener.onError(e);
}
}

private synchronized void handle(TradeSubmitted trade) {
if (isFinished()) {
reject(trade);
return;
}

if (wrongCounterParty(trade)) {
skip(trade);
} else {

You may be able reduce the amount of synchronisation, but you cannot eliminate it entirely. CAS or Fetch-And-Add are lower level operations that are abstracted away by Java so you cannot use them directly.

Firstly, some observations: due to the mechanisms of how the lock is acquired, your synchronization of handle will cause a random caller to be selected when multiple are waiting. This could cause a situation where trades are handled out of order, which would be evidenced in the timestamps. If this isn't desired, you should use a queue to ensure trades are processed fairly (although what constitutes fair is a matter of debate).

You shouldn't use your CheckException as a convenient way to skip past a bunch of code. Have creditCheck(...) return a boolean and use proper flow control, otherwise your code appears to accept a trade even if the credit check fails.

Some of your preconditions to a trade may allow you to reject a trade before entering the synchronisation block. If the CounterParty doesn't match, you will always reject a trade. If a trade is above the daily limit, you will reject the trade outright. If a trade is above the trading limit, you can run the credit check without synchronising and reject it if it fails. It also appears that you only ever increment utilisedDailyLimit so if any trade's value would exceed the daily limit (and assuming trade values cannot be negative) you may reject it with an unsynchronised check against the daily limit.

Once you've checked all these preconditions, you have to worry about concurrency. You will have to check the daily limit again, but if this is done on a single-threaded consumer thread, you do not have to synchronise at all because there is no concurrent access and you know the value cannot change. With this approach, may also wish to submit the eventListener.accepted(...) call to a thread pool to execute separately as this may also slow down execution calling the listener inline.

A BlockingQueue would be ideal for this kind of design.

StackExchange Code Review Q#78297, answer score: 3