Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add DiskShare#mkdirs #627

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add DiskShare#mkdirs #627

wants to merge 1 commit into from

Conversation

alb-i986
Copy link

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
Copy link

bangert commented Mar 9, 2022

@alb-i986
Copy link
Author

alb-i986 commented Mar 13, 2022

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants