FileBufferedInterceptor.dispose not working due to locked file

[joakime]

Jetty version(s) 9.4.x - HEAD

Java version/vendor (use: java -version) Java 8u292 and 11.0.1

OS type/version Windows 10

Description The FileBufferedInterceptor.dispose() attempts to clean up files it has recently used. However, this is not possible currently on windows, as it will throw a AccessDeniedException due to the fact that the file is currently locked.

We need to verify that we properly close the various file handles and resources for these files before we call dispose.

2021-07-27 16:03:45.670:WARN:oejsh.FileBufferedResponseHandler:qtp2144353034-5103: Could not delete file C:\Users\jenkins\workspace\nightlies\jetty-9.4.x-windows-nightly\jetty-server\target\tests\test-org.eclipse.jetty.server.handler.FileBufferedResponseHandlerTest\BufferedResponse15180402607277205671
java.nio.file.AccessDeniedException: C:\Users\jenkins\workspace\nightlies\jetty-9.4.x-windows-nightly\jetty-server\target\tests\test-org.eclipse.jetty.server.handler.FileBufferedResponseHandlerTest\BufferedResponse15180402607277205671
    at java.base/sun.nio.fs.WindowsException.translateToIOException(WindowsException.java:89)
    at java.base/sun.nio.fs.WindowsException.rethrowAsIOException(WindowsException.java:103)
    at java.base/sun.nio.fs.WindowsException.rethrowAsIOException(WindowsException.java:108)
    at java.base/sun.nio.fs.WindowsFileSystemProvider.implDelete(WindowsFileSystemProvider.java:274)
    at java.base/sun.nio.fs.AbstractFileSystemProvider.delete(AbstractFileSystemProvider.java:105)
    at java.base/java.nio.file.Files.delete(Files.java:1141)
    at org.eclipse.jetty.server.handler.FileBufferedResponseHandler$FileBufferedInterceptor.dispose(FileBufferedResponseHandler.java:124)

How to reproduce? Run the build / test on windows 10.

[joakime]

After a conversation with @sbordet we are going to use the Java 9+ JVM feature java.lang.ref.Cleaner on Jetty 10+ to cleanup temp files in FileBufferedHandler properly for all OS's, including Linux.

[lachlan-roberts]

@joakime @sbordet I think we already looked into doing this before and determined it was not possible, see issue #6046.

[joakime]

@sbordet and I tested this, we think we have a solution specific for FileBufferedInterceptor, but only on Jetty 10.0.x+

[sbordet]

@lachlan-roberts in #6046 I wanted to deallocate the memory explicitly by invoking an API. This is what sun.misc.Cleaner.clean() was doing for e.g. DirectByteBuffer.

However, turns out that java.lang.ref.Cleanable.clean() has a completely different semantic and it's not possible to use it for #6046.

However, it is possible to do unrelated actions (such as close a File), which is what @joakime is doing to fix this issue.

[lachlan-roberts]

@sbordet And we can't just do this in the callback because the MappedByteBuffers are somehow holding the file open until they are garbage collected?

[gregw]

How about we just don't use filemapped buffers on windows?

[sbordet]

@sbordet And we can't just do this in the callback because the MappedByteBuffers are somehow holding the file open until they are garbage collected?

Correct.

With sun.misc.Cleaner.clean() we could have released the mapped buffer immediately, but we cannot use the API, not even via reflection (due to JPMS).

With java.lang.ref.Cleanable.clean() we have to wait for the GC to release the mapped buffer, and then we can close the file.

How about we just don't use filemapped buffers on windows?

Not a bad idea 😃

[joakime]

How about we just don't use filemapped buffers on windows?

That would mean everywhere we currently use BufferUtil.toMappedBuffer would need to be reviewed.

In jetty-10.0.x, that's a short list ...

• org.eclipse.jetty.server.CachedContentFactory

https://github.com/eclipse/jetty.project/blob/49a08450c2f03e4ed4a3c81866274735ba7c97ee/jetty-server/src/main/java/org/eclipse/jetty/server/CachedContentFactory.java#L337-L352

• org.eclipse.jetty.server.handler.FileBufferedResponseHandler

https://github.com/eclipse/jetty.project/blob/49a08450c2f03e4ed4a3c81866274735ba7c97ee/jetty-server/src/main/java/org/eclipse/jetty/server/handler/FileBufferedResponseHandler.java#L202-L213

[joakime]

We also have usages of java.nio.channels.FileChannel.map() directly to review.

https://github.com/eclipse/jetty.project/blob/49a08450c2f03e4ed4a3c81866274735ba7c97ee/demos/embedded/src/main/java/org/eclipse/jetty/demos/FastFileServer.java#L196-L197

https://github.com/eclipse/jetty.project/blob/49a08450c2f03e4ed4a3c81866274735ba7c97ee/jetty-proxy/src/main/java/org/eclipse/jetty/proxy/AfterContentTransformer.java#L272

https://github.com/eclipse/jetty.project/blob/49a08450c2f03e4ed4a3c81866274735ba7c97ee/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/DataRateLimitedServlet.java#L129

[joakime]

Opened PR #6588 to show proposal of usage.

One wart I don't like, is if the JVM closes before the phantom reference is reached, the registered action does not seem to execute.

[lorban]

@joakime You could work around that problem by restricting the mechanism to only deleting files instead of executing any random job and completing it with File.deleteOnExit.

But I'm not fond of this idea, I think any mechnism that relies on the GC to manage OS resources is doomed to fail soon or late, so I'd rather invest time in Greg's proposal of not using file mapped buffers anymore.

[joakime]

Postponing to 10.0.10

[github-actions[bot]]

This issue has been automatically marked as stale because it has been a full year without activity. It will be closed if no further activity occurs. Thank you for your contributions.

[github-actions[bot]]

This issue has been closed due to it having no activity.