I have a Meeting class that is persisted to DB using JPA hibernate in an Spring Boot Application interfaced by a REST API, I have performance concerns on operations that must be thread safe. This is the Meeting class:
@Entity
public class Meeting {
@Id
@GeneratedValue(strategy= GenerationType.AUTO)
private Long id;
@ManyToOne(optional = false)
@JoinColumn(name = "account_id", nullable = false)
private Account account;
private Integer maxAttendees;
private Integer numAttendees; // current number of attendees
...
}
As you can see I have an Account entity, an account can have many meetings associated. A meeting has a maximum number of attendees and an account has a maximum number of scheduled meetings, in a similar fashion an Account has a maxSchedules and numSchedules variables.
The basic workflow is: A meeting is created, then scheduled, then attendees are registered individually.
Note: The main goal here is avoiding to exceed the maximum number of allowed operations (schedules or registers).
I initially was more focused on the business logic than performance, initially my business service for scheduling and registering attendees looked like this:
@Service
public class MeetingService {
...
@Transactional
public synchronized void scheduleMeeting(Long meetingId, Date meetingDate) {
Meeting meeting = repository.findById(meetingId);
Account account = meeting.getAccount();
if(account.getNumSchedules() + 1 <= account.getMaxSchedules()
&& meeting.getStatus() != SCHEDULED) {
meeting.setDate(meetingDate);
account.setNumSchedules(account.getNumSchedules()+1);
// save meeting and account here
}
else { throw new MaxSchedulesReachedException(); }
}
@Transactional
public synchronized void registerAttendee(Long meetingId, String name) {
Meeting meeting = repository.findById(meetingId);
if(meeting.getNumAttendees() + 1 <= meeting.getMaxAttendees()
&& meeting.getStatus() == SCHEDULED) {
meeting.setDate(meetingDate);
meeting.setNumAttendees(account.getNumAttendees()+1);
repository.save(meeting);
}
else { throw new NoMoreAttendeesException(); }
}
...
}
The problem with this approach is that the lock of the synchronized methods is the object (this), the service is a singleton instance so when multiple threads are trying to perform any of the two synchronized operations, they need to wait for the lock to be released.
I came with a second approach using separated locks for scheduling and registration:
...
private final Object scheduleLock = new Object();
private final Object registerLock = new Object();
...
@Transactional
public void scheduleMeeting(Long meetingId, Date meetingDate) {
synchronized (scheduleLock) {
Meeting meeting = repository.findById(meetingId);
Account account = meeting.getAccount();
if(account.getNumSchedules() + 1 <= account.getMaxSchedules()
&& meeting.getStatus() != SCHEDULED) {
meeting.setDate(meetingDate);
account.setNumSchedules(account.getNumSchedules()+1);
// save meeting and account here
}
else { throw new MaxSchedulesReachedException(); }
}
}
@Transactional
public void registerAttendee(Long meetingId, String name) {
synchronized (registerLock) {
Meeting meeting = repository.findById(meetingId);
if(meeting.getNumAttendees() + 1 <= meeting.getMaxAttendees()
&& meeting.getStatus() == SCHEDULED) {
meeting.setDate(meetingDate);
meeting.setNumAttendees(account.getNumAttendees()+1);
repository.save(meeting);
}
else { throw new NoMoreAttendeesException(); }
}
}
...
With this I solved the inter-operations blocking problem, which means that a thread that wants to register shouldn't be blocked by a thread that is scheduling.
Now the issue is that a thread that is working on scheduling a meeting in one account shouldn't be locked by a thread trying to schedule a meeting from a different account, the same can be said for registering attendees to different meetings in the same account.
For fixing that, I came with a design that I haven't implemented yet but the idea is to have a lock provider, something like:
@Component
public class LockProvider {
private final ConcurrentMap<String, Object> lockMap = new ConcurrentHashMap();
private Object addAccountLock(Long accountId) {
String key = makeAccountKey(accountId);
Object candidate = new Object();
Object existing = lockMap.putIfAbsent(key, candidate);
return (existing != null ? existing : candidate);
}
private Object addMeetingLock(Long accountId, Long meetingId) {
String key = makeMeetingKey(accountId, meetingId);
Object candidate = new Object();
Object existing = lockMap.putIfAbsent(key, candidate);
return (existing != null ? existing : candidate);
}
private String makeAccountKey(Long accountId) {
return "acc"+accountId.toString();
}
private String makeMeetingKey(Long accountId, Long meetingId) {
return "meet"+accountId.toString()+meetingId.toString();
}
public Object getAccountLock(Long accountId) {
return addAccountLock(accountId);
}
public Object getMeetingLock(Long accountId, Long meetingId) {
return addMeetingLock(accountId, meetingId);
}
}
However this approach involves lot of extra work on maintaining the map, for example making sure to dispose no longer used locks when an account, meeting is deleted or they come to a state where no more synchronized operations can be accomplished.
The question is if it is worth implementing that or if there is a more efficient way of doing it.
