Race condition in creating directories on PUT

[paulmillar]

PutHandler contains the following code:

                Resource r = parent.child(path.getName());

                if (r == null) {
                        log.info("Could not find child: " + path.getName() + " in parent: " + parent.getName() + " - " + parent.getClass());
                        if (parent instanceof MakeCollectionableResource) {
                                MakeCollectionableResource mkcol = (MakeCollectionableResource) parent;
                                if (!handlerHelper.checkAuthorisation(manager, mkcol, request)) {
                                        throw new NotAuthorizedException(mkcol);
                                }
                                log.info("autocreating new folder: " + path.getName());
                                CollectionResource newCol = mkcol.createCollection(path.getName());
                                manager.getEventManager().fireEvent(new NewFolderEvent(newCol));
                                return newCol;
                        } else {
                                log.info("parent folder isnt a MakeCollectionableResource: " + parent.getName() + " - " + parent.getClass());
                                return null;
                        }
                } else if (r instanceof CollectionResource) {

The problem is that, if there are many concurrent requests targeting the same non-existing path then it is possible for a collection that does not exist for parent.child(path.getName()) to be created by the time mkcol.createCollection(path.getName()) is called.

RFC 4918 is a little vague on how MKCOL should react if the entity already exists: it says the request must fail, but does not indicate with which status code. I believe 400 Bad Request (i.e., BadRequestException) is the expected response, despite not being listed in RFC 4918 § 9.3.1. The MakeCollectionableResource interface supports BadRequestException but the JavaDoc doesn't describe the expectation (i.e., when this exception is to be thrown).

Assuming BadRequestException is the expected reaction, one way of fixing this would be to catch this exception and retry the parent.child(path.getName()) request.

[paulmillar]

Note that the same problem exists in MkColHandler:

		Resource existingChild = existingCol.child(newName);
		if (existingChild != null) {
		        // ...
			responseHandler.respondMethodNotAllowed(existingChild, response, request);
			return;
		}
		CollectionResource made = creator.createResource(existingCol, newName, request);

Multiple concurrent MKCOL requests targeting the same missing collection could all see a null value for existingChild, so multiple threads will attempt to call CollectionResourceCreator#createResource, which (by default), calls MakeCollectionableResource#createCollection.

Interestingly, the code here returns status code 405 Method Not Allowed (which I see now is actually the correct code).

Therefore, I believe the correct solution here is to add a new throwable exception in the MakeCollectionableResource#createCollection method's signature to indicate the resource already exists, and update MkColHandler and PutHandler to react correctly to that exception.

[bradmac]

Assuming BadRequestException is the expected reaction, one way of fixing this would be to catch this exception and retry the parent.child(path.getName()) request.

I think that would be exceeding the scope of milton's functionality. Milton isnt responsible for transaction handling/isolation etc.

Concurrency is intended to be handled in webdav through locking. Most operating system clients use an elaborate sequence of actions to ensure concurrency, eg create a zero byte file with a temp name, lock it, upload to it, then move it to the desired name.

In addition there is a feature defined in the webdav spec referred to as the 'lock null' process where a resource can be locked before it exists. Milton fully supports lock-null, but i understand Mac Finder is the only OS client which supports it.

[bradmac]

I think if you have an implementation with concurrency problems the best way to handle it is

• ensure you support lock-null (if Finder is a supported client)
• hold a list of pending resource creation operations, and use that to validate resource names when a create request is received. This in effect intentionally breaks transaction isolation for this case.

[paulmillar]

Thanks for the reply, but I believe we are talking somewhat at cross-purposes.

I appreciate that WebDAV support locking; however here I am interested in how Milton supports multiple clients, each issuing a single PUT request.

To give you a concrete example: I discovered this problem when attempting to use JMeter to benchmark performance characteristics. My simple test-case has JMeter with a threadgroup (=> simulating multiple clients), where each is uploading a unique file: thread-1 uploads to /public/jmeter/thread-1.dat, thread-2 uploads to /public/jmeter/thread-2.dat, etc.

Each client issues a PUT request at (more or less) the same time, as the test-case starts. This means that the jetty server will process these PUT requests will concurrently, using different threads.

The directory /public/jmeter does not exist before the test starts. Multiple jetty/milton threads notice parent.child(path.getName()) returning null (for a /public parent and jmeter as the name). Therefore, these multiple threads all call createCollection. Naturally, only the first thread will succeed, and all other threads will fail.

Thanks for the hint about lock-null -- I'll check that dCache supports this.

However, we have clients that do not lock all resources in the path (/, /public /public/jmeter, /public/jmeter/thread-1.dat) before issuing a PUT request, so I believe the lock-null support would likely have limited effect.

I also don't believe your second solution really solves the problem for a few reasons:

1. holding the list of pending creations doesn't fix the race condition, it just reduces the window (makes it less likely)
2. a typical production deployment has multiple servers. This list of pending creations would need to be synchronised across all such servers. Achieving agreement reliably across multiple servers is expensive in terms of performance (see algorithms like PAXOS, RAFT; and software like ZooKeeper).

My proposed solution is relatively simple and backwards compatible: update the MakeCollectionableResource interface to allow the method to throw an exception if the resource already exists.

[bradmac]

I think i do understand what you mean. But the example above is invalid. For your JMeter script to be correct it should use locking as dav clients are required to do.

Milton's semantics are correct, in that if a resource exists we respond with a 405 as above. Milton tries to take on the burden of complying with protocol requirements rather then imposing that on implmentation developers. So we want to retain the current functionality where we check to see if a resource exists before calling the createCollection method.

I dont believe that persisting state across clusters is impractical. There are many good tools for doing this. In fact you need to do this with locks (including lock-null), and you should do it for nonces, for milton to work correctly in a clustered environment.

So to summarise

• i think its reasonable for a server to assume clients will correctly implement locking
• and you can optimise your solution by maintaining a list of in-progress, but uncommited, operations.

[bradmac]

Also, you can configure the milton stack to use your own MkColHandler, which could remove the check to see whether the resource exists. Milton is designed to be pluggable for this reason.