You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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 (SMBApiExceptione) {
if (e.getStatus() != NtStatus.STATUS_OBJECT_NAME_COLLISION) { // skip for concurrency issuesthrowe;
}
}
}
That's ok but still it's not the best.
The best is to have #mkdirnot 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:
I'm not familiar with Spock, although this might be a good opportunity for me to learn :)
I tried to write a standard JUnit4 test class but I couldn't get it to run
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:
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:
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I bumped into a race condition while using the existing
DiskShare#mkdirWhat happened was that I had 3 threads executing the following client code:
This is a method I wrote myself in order to deal with the fact that
#mkdirthrows 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:
That's ok but still it's not the best.
The best is to have
#mkdirnot throw in the first place, when the directory already exists.So here I'm proposing to add a
#mkdirsmethod 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:
#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: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:
Path#resolve), joining paths (it's so annoying having to dopath + "\\" fileall 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
#mkdirscould look like: