Closed dzaima closed 1 year ago
Great addition! Do you think ATOMIC_MOVE can fail though? And REPLACE_EXISTING would work in that case? Maybe we should just try ATOMIC_MOVE and if it fails, die? Basically delete everything that is in first catch block? Also, I see you capture exception and continue, wouldn’t it be better to die there?
Actually, it seems I didn't properly test the fallback - it'll try to read the InputStream
twice, which doesn't work.
Do you think ATOMIC_MOVE can fail though? And REPLACE_EXISTING would work in that case? Maybe we should just try ATOMIC_MOVE and if it fails, die?
The docs say that ATOMIC_MOVE
may not be supported on some file systems/OSes/something; possible that it doesn't affect anything Skija targets, but I don't know.
Also, on a more careful read, an IOException
may be thrown if the file already existed (i.e. another Skija just moved to it), so this definitely needs some reworking. Working on it.
Pushed a slightly simpler approach - always extract just to the temporary file, try moving with ATOMIC_MOVE
, and fall back to an equivalent REPLACE_EXISTING
move if that threw an exception and didn't produce the result file.
As a side-note, doing this seems to improve the reliability of the fallback path too - haven't observed it failing. (possible it's just also done atomically on my system, but even if not, moving the decompression out of the move probably reduces the latency and thus possibility of it being done by multiple processes at the same time)
Can you check the solution I just merged? I have a feeling just ATOMIC_MOVE should be enough, given that it’s in the same folder. I also used createTempFile for uniqueness guarantee
Fails with
java.lang.ExceptionInInitializerError
at dzaima.ui.gui.Typeface.<clinit>(Typeface.java:18)
at dzaima.ui.gui.config.GConfig.cfgUpdated(GConfig.java:44)
at dzaima.ui.gui.config.GConfig.reloadCfg(GConfig.java:64)
at dzaima.ui.gui.config.GConfig.newConfig(GConfig.java:92)
at dzaima.ui.apps.ExMain.run(ExMain.java:27)
at dzaima.ui.gui.Windows.lambda$start$0(Windows.java:53)
at io.github.humbleui.jwm.App.lambda$start$1(App.java:35)
at io.github.humbleui.jwm.App._nStart(Native Method)
at io.github.humbleui.jwm.App.start(App.java:28)
at dzaima.ui.gui.Windows.start(Windows.java:53)
at dzaima.ui.apps.ExMain.main(ExMain.java:69)
Caused by: java.io.IOException: No such file or directory
at java.base/java.io.UnixFileSystem.createFileExclusively0(Native Method)
at java.base/java.io.UnixFileSystem.createFileExclusively(UnixFileSystem.java:356)
at java.base/java.io.File.createTempFile(File.java:2179)
at io.github.humbleui.skija.impl.Library._extract(Library.java:165)
at io.github.humbleui.skija.impl.Library.load(Library.java:101)
at io.github.humbleui.skija.impl.Library.staticLoad(Library.java:20)
at io.github.humbleui.skija.FontMgr.<clinit>(FontMgr.java:7)
... 11 more
tempDir.mkdirs();
should be moved before File.createTempFile
Right, check again plz
java.lang.ExceptionInInitializerError
at dzaima.ui.gui.Typeface.<clinit>(Typeface.java:18)
at dzaima.ui.gui.config.GConfig.cfgUpdated(GConfig.java:44)
at dzaima.ui.gui.config.GConfig.reloadCfg(GConfig.java:64)
at dzaima.ui.gui.config.GConfig.newConfig(GConfig.java:92)
at dzaima.ui.apps.ExMain.run(ExMain.java:27)
at dzaima.ui.gui.Windows.lambda$start$0(Windows.java:53)
at io.github.humbleui.jwm.App.lambda$start$1(App.java:35)
at io.github.humbleui.jwm.App._nStart(Native Method)
at io.github.humbleui.jwm.App.start(App.java:28)
at dzaima.ui.gui.Windows.start(Windows.java:53)
at dzaima.ui.apps.ExMain.main(ExMain.java:69)
Caused by: java.nio.file.FileAlreadyExistsException: /tmp/skija_0.109.3_x64/libskija.so9527711736127876018tmp
at java.base/sun.nio.fs.UnixException.translateToIOException(UnixException.java:94)
at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:106)
at java.base/sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:111)
at java.base/sun.nio.fs.UnixFileSystemProvider.newByteChannel(UnixFileSystemProvider.java:218)
at java.base/java.nio.file.spi.FileSystemProvider.newOutputStream(FileSystemProvider.java:484)
at java.base/java.nio.file.Files.newOutputStream(Files.java:228)
at java.base/java.nio.file.Files.copy(Files.java:3159)
at io.github.humbleui.skija.impl.Library._extract(Library.java:175)
at io.github.humbleui.skija.impl.Library.load(Library.java:101)
at io.github.humbleui.skija.impl.Library.staticLoad(Library.java:20)
at io.github.humbleui.skija.FontMgr.<clinit>(FontMgr.java:7)
... 11 more
needs a REPLACE_EXISTING
on the Files.copy
, as File.createTempFile
already creates an (empty) file
With that additional REPLACE_EXISTING
edited in locally, does pass my testing (that being linux only)
@tonsky you didn't ever added that other REPLACE_EXISTING
, i.e. Files.copy(is, fileTmp.toPath());
→ Files.copy(is, fileTmp.toPath(), StandardCopyOption.REPLACE_EXISTING);
; without that, it's still broken at least on linux (after a rm -r /tmp/skija_0.116.0_x64
, which is what a fresh install would have)
But it’s a temp file, it’s always new and non-existent, not?
File.createTempFile
by itself already creates the file, though (initializing it to empty contents).
Yes, of course! Sorry about that. Pushed 0.116.1, should be available soon
Fix for #54
Fixes the issue for me, but I've only tested on Linux (though, at worst, it should just do the fallback implementation that's the same as what was done before).
Done via extracting to a temporary file with a unique name, and doing an atomic move to the proper destination. Thus, the proper
/tmp/skija_0.123.4_x64/libskija.so
may be changed by multiple processes in parallel, but it should always be in a valid state.