Add DiskShare#mkdirs

[alb-i986]

I bumped into a race condition while using the existing DiskShare#mkdir

com.hierynomus.mssmb2.SMBApiException: STATUS_OBJECT_NAME_COLLISION (0xc0000035): Create failed for \\path\to\dir
    at com.hierynomus.smbj.share.Share.receive(Share.java:371)
    at com.hierynomus.smbj.share.Share.sendReceive(Share.java:351)
    at com.hierynomus.smbj.share.Share.createFile(Share.java:159)
    at com.hierynomus.smbj.share.DiskShare.createFileAndResolve(DiskShare.java:97)
    at com.hierynomus.smbj.share.DiskShare.resolveAndCreateFile(DiskShare.java:79)
    at com.hierynomus.smbj.share.DiskShare.open(DiskShare.java:66)
    at com.hierynomus.smbj.share.DiskShare.openDirectory(DiskShare.java:130)
    at com.hierynomus.smbj.share.DiskShare.mkdir(DiskShare.java:253)

What happened was that I had 3 threads executing the following client code:

if (!share.folderExists(dirPath)) {
  share.mkdir(dirPath);
}

This is a method I wrote myself in order to deal with the fact that #mkdir throws an exception when the directory already exists.

But it was flawed, as the operation was not atomic.

A better way to deal with this problem could have been to silent the exception in case the status is STATUS_OBJECT_NAME_COLLISION:

if (!share.folderExists(dirPath)) {
    try {
        share.mkdir(dirPath);
    } catch (SMBApiException e) {
        if (e.getStatus() != NtStatus.STATUS_OBJECT_NAME_COLLISION) { // skip for concurrency issues
            throw e;
        }
    }
}

That's ok but still it's not the best.

The best is to have #mkdir not throw in the first place, when the directory already exists.

So here I'm proposing to add a #mkdirs method along with #mkdir, with the same semantics as Files#createDirectories (I even copied its Javadoc :) )

I didn't write tests for a couple of reasons:

1. I'm not familiar with Spock, although this might be a good opportunity for me to learn :)
2. I tried to write a standard JUnit4 test class but I couldn't get it to run
3. I think it's not by chance that I couldn't find any unitests for DiskShare. It's because it's not testable. In my opinion, the best it would be to have #mkdirs, together with any other method providing high level, client-facing functionality (e.g. SmbFiles#copy, which could be very useful to users; it's unfortunate it is so "hidden"), in a Facade class which depended on a lower level, mockable class. So then it would be possible to write a simple test with mocks like this one:

@Test
public void shouldCreateAllParentDirs() throws IOException {
    SmbjFacade sut = new SmbjFacade(wrapperMock);
    sut.mkdirs("root\\subdir\\subsubdir");

    verify(wrapperMock).mkdir("root");
    verify(wrapperMock).mkdir("root" + "\\" + "subdir");
    verify(wrapperMock).mkdir("root" + "\\" + "subdir" + "\\" + "subsubdir");
}

Also (but this should be another topic actually), I think it would be good if the path arguments we take from the user were of type Path. It would make our lives easier:

• not having to deal with invalid input, only null checks
• being able to decompose a path in its components, resolving paths from a parent (Path#resolve), joining paths (it's so annoying having to do path + "\\" file all the time, isn't it? ;) )

Plus, it would make sense from a domain point of view: we're really dealing with paths!

As an example, this is how my #mkdirs could look like:

public void mkdirs(Path path) {
        Iterator<Path> pathIt = path.iterator();
        if (!pathIt.hasNext()) {
            return;
        }
        Path parent = pathIt.next();
        mkdirQuietly(parent);
        while (pathIt.hasNext()) {
            Path dir = parent.resolve(pathIt.next());
            mkdirQuietly(dir);
            parent = dir;
        }
    }

[bangert]

@alb-i986 how does your solution compare to SmbFiles.mkdirs ? https://github.com/hierynomus/smbj/blob/master/src/main/java/com/hierynomus/smbj/utils/SmbFiles.java#L79

[alb-i986]

@alb-i986 how does your solution compare to SmbFiles.mkdirs ? https://github.com/hierynomus/smbj/blob/master/src/main/java/com/hierynomus/smbj/utils/SmbFiles.java#L79

The problem with SmbFiles.mkdirs is that it's not atomic. So you would still incur the concurrency issue which this PR fixes.

See what I wrote in my original post:

What happened was that I had 3 threads executing the following client code:

if (!share.folderExists(dirPath)) {
  share.mkdir(dirPath);
}

This is a method I wrote myself in order to deal with the fact that #mkdir throws an exception when the directory already exists. But it was flawed, as the operation was not atomic.