AwsClientBuilder is not thread safe

[bogovicj]

AwsClientBuilder is not thread safe, and creating multiple N5Readers / N5Writers in parallel could run into this.

This could be the cause of intermittent test failures that reach out to s3.

@cmhulbert @tischi

[cmhulbert]

Nice find! Since the refactor, it is actually only ever called in one place. Synchronizing on that method should be simple enough.

That said, trying to reproduce the error minimally in n5-aws-s3 so far has not triggered the failure for me. For example, this passes without exception:

	@Test
	public void parallelS3Creation() throws InterruptedException, ExecutionException {

		List<Callable<AmazonS3>> tasks = new ArrayList<>();
		for (int i = 0; i < 1_000; i++) {
			final int idx = i;
			tasks.add(() -> {
				final String uri = "s3://n5-test-erroneous-no-such-bucket/test_container-" + idx + ".n5";
				final AmazonS3 s3 = AmazonS3Utils.createS3(uri);
				return s3;
			});
		}
		final ExecutorService executor = Executors.newFixedThreadPool(Runtime.getRuntime().availableProcessors());
		final List<Future<AmazonS3>> futures = executor.invokeAll(tasks);
		for (Future<AmazonS3> future : futures) {
			future.get();
		}

	}

Regarding the issue I mentioned in zullip, the erroneaous No Such Bucket exception, I think that is not cause by this issues though, since I was able to trigger it locally with something along the lines of this test:

        @Test
	public void erroneousErrorTest() {

		final String bucket = "n5-test-erroneous-no-such-bucket";
		final String container = "test_container.n5/";
		final String uri = "s3://" + bucket + "/" + container;
		final AmazonS3 s3 = AmazonS3Utils.createS3(uri); // the only call to AwsS3ClientBuilder
		s3.createBucket(bucket);
		s3.putObject(bucket, container, "");
		s3.putObject(bucket, container + "attributes.json", "{version: \"1.2.3\"}");
		assertTrue(s3.doesBucketExistV2(bucket));
		for (int i = 0; i < 1_000; i++) {
			s3.listObjectsV2(bucket);
		}
	}

That said, when I'm currently trying to trigger the issue with the above test, I'm not able to 🤷‍♀️ . But this is "consistent" with the sporadic nature of the exception. I'm not overly concerned about that issues though, it seems rare enough, even when intentionally trying to trigger it.

[bogovicj]

Update after investigation:
That AwsClientBuilder is marked as not thread safe was not the cause of the issues.

We may still want to guard against potential concurrency issues, but so far its not clear that a such a problem has ever been observed.

[cmhulbert]

@axtimwalde replying here, now that the zullip issue is resolved

One thing that isn't clear to me is what the @NotThreadSafe annotation means. It's doc states:

Documenting annotation to indicate a class is not thread-safe and should not be used in a multi-threaded context.

I'm not sure if this implies that it's not safe to use:

1. the same instance of an AmazonS3ClientBuilder across multiple threads (we don't do this); or;
2. unique instances per thread (this could happen)

Nothing I have found documentation wise seems to clarify this so far, and looking at the code, it doesn't seem to me that what we currently are doing (the second point) should not be thread safe.