Synchronisation issues

[alebastrov]

1. createBucket & loadBuckets methods - vulnerability time gap

lockStore.putIfAbsent(bucketName, new Object());
synchronized (lockStore.get(bucketName)) ...

there SHOULD be

var newObj = new Object();
var obj = lockStore.putIfAbsent(bucketName, newObj);
synchronized (obj == null ? newObj : obj) ...

2. var bucketMetadata = getBucketMetadata(bucketName); method is being called from sync and out of sync code which is weird, I'd call this method in all places FROM synchronization block

3. putObject - 2 operations (check + create) with 'bucketMetadata' leads to vulnerability time gap but they (couple of operations) entirely should be atomic (check&create at once)

var bucketMetadata = bucketStore.getBucketMetadata(bucketName);
    var id = bucketMetadata.getID(key);
    if (id == null) {
      id = bucketStore.addToBucket(key, bucketName);
    }

consider of replacing them to atomic 'addIfAbsent(key, bucketName)' - in bucketStore it should be sync method

4. deleteObject -the same as 3) - bucketStore - get/remove should be atomic

    var bucketMetadata = bucketStore.getBucketMetadata(bucketName);
    var id = bucketMetadata.getID(key);
    if (id == null) {
      return false;
    }

    if (objectStore.deleteObject(bucketMetadata, id)) {
      return bucketStore.removeFromBucket(key, bucketName);
    } else {
      return false;
    }

it's better to replace them to 'deleteIfExist(bucketName, id)'

5. other methods in ObjectService, like setObjectTags/copyS3Object/putS3Object/setObjectTags/setLegalHold etc

[afranken]

@alebastrov
thanks for looking at ways to improve our code.

This application is not meant to be used with massive parallel requests manipulating the same objects or buckets, but for integration testing services.

APIs like setLegalHold and others would only fail if the object is deleted while the API is being called.
Again, this is meant for testing your service against a local mock of S3. Not sure why your integration test would delete objects or buckets while manipulating them with another connection...

[alebastrov]

Our tests are based on idea that some thread puts object to s3, another one reads/verifies whether it exists. Sometimes that scenario fails due to illegal response - object does not exist or put object on s3 fails

[afranken]

are the threads putting / modifying the same resources over and over?
What is the actual use-case for this?

[alebastrov]

yep. resources names are always the same and issue appears when NO parallel requests are coming

[afranken]

yep. resources names are always the same and issue appears when NO parallel requests are coming

please provide a reproducer for this.
There are integration-tests passing on every commit that concurrently modify resources and buckets.

[afranken]

closing for lack of activity and evidence.