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.

Sign up for GitHub

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

Jump to bottom

Resources: closing outputstream bug, recursive delete #1176

Closed
wants to merge 1 commit into from
Closed

Resources: closing outputstream bug, recursive delete #1176

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 11 additions & 8 deletions src/platform/src/main/java/org/geoserver/platform/resource/FileSystemResourceStore.java
View file
Open in desktop
Expand Up @@ -228,13 +228,16 @@ public OutputStream out() {
@Override
public void close() throws IOException {
delegate.close();
Lock lock = lock();
try {
// no errors, overwrite the original file
Files.move(temp, actualFile);
}
finally {
lock.release();
//if already closed, there should be no exception (see spec Closeable)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is tricky logic, would love to know the file was written out at lest once :) Thinking about a resource against a directory with no write access. The tmp file would be created, the move would fail, and each successive close would produce the same error ...

You can decide if you care.

if (temp.exists()) {
Lock lock = lock();
try {
// no errors, overwrite the original file
Files.move(temp, actualFile);
}
finally {
lock.release();
}
}
}

Expand Down Expand Up @@ -429,7 +432,7 @@ public String toString() {

@Override
public boolean delete() {
return file.delete();
return file.exists() && Files.delete(file);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about this one, the functionality was isolated into the Resources class - specifically so implementors (like you) would have less work to do when implementing a Resource.

See: That functionality was already accounted for ...

The functionality is factored out into the Resources class here:

https://github.com/geoserver/geoserver/blob/master/src/platform/src/main/java/org/geoserver/platform/resource/Resources.java#L290

If you are going to change the method contract you will need to update the Resource.delete() javadoc to explain what you are about, and remove the Resources.delete( Resource ) method as it will now be useless.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is a bad idea, because deleting recursively can happen a lot more efficiently in a JDBC database (with a lot less queries than would happen if you call the Resources.delete method)

Note that there is also a store.remove() function which is recursive (and returns true if the resource didn't exist yet - which is also a contradiction).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your feedback is a bit late, we can take it to geotools-devel. The issue with this one is you combined a good fix (that needed to be back ported) with an API change which did not - so I had to split it up.


@Override
Expand Down
5 changes: 4 additions & 1 deletion src/platform/src/main/java/org/geoserver/platform/resource/Files.java
View file
Open in desktop
Expand Up @@ -105,7 +105,10 @@ public OutputStream out() {
@Override
public void close() throws IOException {
delegate.close();
Files.move(temp, file);
//if already closed, there should be no exception (see spec Closeable)
if (temp.exists()) {
Files.move(temp, file);
}
}

@Override
Expand Down
6 changes: 6 additions & 0 deletions ...platform/src/test/java/org/geoserver/platform/resource/FileWrapperResourceTheoryTest.java
View file
Open in desktop
Expand Up @@ -111,6 +111,12 @@ public void theoryAddingFileToDirectoryAddsResource(String path)
throws Exception {

}

@Override @Ignore
public void theoryRecursiveDelete(String path)
throws Exception {

}

// paths for file wrapper are special so ignore this test
@Override @Ignore
Expand Down
23 changes: 23 additions & 0 deletions src/platform/src/test/java/org/geoserver/platform/resource/ResourceTheoryTest.java
View file
Open in desktop
Expand Up @@ -486,4 +486,27 @@ public void theoryMultipleOutputStreamsAreSafe(String path) throws Exception {
assertThat(resultContent, anyOf(equalTo(thread1Content), equalTo(thread2Content)));

}

@Theory
public void theoryDoubleClose(String path) throws Exception {
final Resource res = getResource(path);
assumeThat(res, is(resource()));

OutputStream os = res.out();
os.close();
os.close();
}

@Theory
public void theoryRecursiveDelete(String path) throws Exception {
final Resource res = getResource(path);
assumeThat(res, is(directory()));
assumeThat(res, is(directory()));

Collection<Resource> result = res.list();
assumeThat(result.size(), greaterThan(0));

assertTrue(res.delete());

}
}