Skip to content

Commit 23707d3

Browse files
committed
fix(dynamodb): harden transaction ordering and drop dangling plan references
Addresses review feedback on floci-io#572: * Replace the delimited-string ordering key for transactWriteItems with a Comparator over TransactParticipant tuples. User-supplied PK/SK bytes can contain any character, so "storageKey + \0 + itemKey" is not an injective ordering — tuple comparison avoids the collision risk entirely. * Release a table's item locks alongside itemsByTable / itemStore in deleteTable so the registry tracks the table's lifetime instead of growing unboundedly across create/drop cycles. * Remove Javadoc and inline comment references to PLAN-dynamodb-concurrency.md, which was a local working doc and was never committed. Issue floci-io#571 stays as the public reference.
1 parent 38b52da commit 23707d3

3 files changed

Lines changed: 16 additions & 8 deletions

File tree

compatibility-tests/sdk-test-java/src/test/java/com/floci/test/DynamoDbConcurrencyTest.java

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,8 +43,7 @@
4343
* End-to-end concurrency tests against a running Floci instance.
4444
*
4545
* <p>Covers the four scenarios that must match real DynamoDB's per-item
46-
* linearisability and transaction atomicity — see issue #571 and
47-
* PLAN-dynamodb-concurrency.md.
46+
* linearisability and transaction atomicity — see issue #571.
4847
*
4948
* <p>Each scenario uses a shared {@link CountDownLatch} starting gate so
5049
* client threads dispatch simultaneously, maximising contention at the server.

src/main/java/io/github/hectorvent/floci/services/dynamodb/DynamoDbService.java

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import java.time.Instant;
2121
import java.util.ArrayList;
2222
import java.util.Collections;
23+
import java.util.Comparator;
2324
import java.util.HashMap;
2425
import java.util.Iterator;
2526
import java.util.List;
@@ -40,8 +41,8 @@ public class DynamoDbService {
4041
// Items stored per table: storageKey -> Map<itemKey, item>
4142
// itemKey is "pk" or "pk#sk" depending on table schema
4243
private final ConcurrentHashMap<String, ConcurrentSkipListMap<String, JsonNode>> itemsByTable = new ConcurrentHashMap<>();
43-
// Per-item locks: storageKey -> itemKey -> ReentrantLock. See PLAN-dynamodb-concurrency.md.
44-
// Locks are created lazily on first access and never removed; transactWriteItems
44+
// Per-item locks: storageKey -> itemKey -> ReentrantLock. Locks are created lazily
45+
// on first access and cleared with the table (see deleteTable); transactWriteItems
4546
// relies on ReentrantLock's re-entrancy so the inner put/update/delete calls do
4647
// not deadlock after the outer transaction already took each participant's lock.
4748
private final ConcurrentHashMap<String, ConcurrentHashMap<String, ReentrantLock>> itemLocks = new ConcurrentHashMap<>();
@@ -209,6 +210,7 @@ public void deleteTable(String tableName, String region) {
209210
}
210211
tableStore.delete(storageKey);
211212
itemsByTable.remove(storageKey);
213+
itemLocks.remove(storageKey);
212214
if (itemStore != null) {
213215
itemStore.delete(storageKey);
214216
}
@@ -670,12 +672,15 @@ public void transactWriteItems(List<JsonNode> transactItems, String region) {
670672
// order before evaluating conditions or applying writes. Total-ordered acquisition
671673
// prevents deadlock across concurrent transactions; ReentrantLock lets the inner
672674
// putItem/updateItem/deleteItem calls re-enter the same lock for free.
673-
TreeMap<String, ReentrantLock> toAcquire = new TreeMap<>();
675+
//
676+
// Ordering uses a tuple comparator — not a delimited string — so user-supplied
677+
// bytes in an item's PK/SK value cannot collide two distinct participants
678+
// into the same ordering key.
679+
TreeMap<TransactParticipant, ReentrantLock> toAcquire = new TreeMap<>(PARTICIPANT_ORDER);
674680
for (JsonNode transactItem : transactItems) {
675681
TransactParticipant p = resolveParticipant(transactItem, region);
676682
if (p == null) continue;
677-
String orderingKey = p.storageKey + "\0" + p.itemKey;
678-
toAcquire.putIfAbsent(orderingKey, lockFor(p.storageKey, p.itemKey));
683+
toAcquire.putIfAbsent(p, lockFor(p.storageKey, p.itemKey));
679684
}
680685

681686
List<ReentrantLock> acquired = new ArrayList<>(toAcquire.size());
@@ -737,6 +742,10 @@ public void transactWriteItems(List<JsonNode> transactItems, String region) {
737742

738743
private record TransactParticipant(String storageKey, String itemKey) {}
739744

745+
private static final Comparator<TransactParticipant> PARTICIPANT_ORDER =
746+
Comparator.comparing(TransactParticipant::storageKey)
747+
.thenComparing(TransactParticipant::itemKey);
748+
740749
private TransactParticipant resolveParticipant(JsonNode transactItem, String region) {
741750
JsonNode target;
742751
boolean isPut = false;

src/test/java/io/github/hectorvent/floci/services/dynamodb/DynamoDbConcurrencyIntegrationTest.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@
3838
* regression that surfaces intermittently has a real chance of being observed in a
3939
* single CI run.
4040
*
41-
* <p>See issue #571 and PLAN-dynamodb-concurrency.md.
41+
* <p>See issue #571.
4242
*/
4343
class DynamoDbConcurrencyIntegrationTest {
4444

0 commit comments

Comments
 (0)