storage.js: check fsync capability from return code rather than using process.platform heuristics #20
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.
This pull request was originally filed against JamesMGreene/nestdb
Why:
Currently fsync on the directory containing the database is skipped if
process.platform
equalswin32
orwin64
. This check is considered insufficient and should be replaced by checks evaluating the error responses of operations, as there exists more than the default Linux (and possibly assumed posix compatible) and Windows filesystem backend/driver/adapter implementations.An Example for (multiple) implementations of filesystem backends is BrowserFS, where most filesystems are not backed by physical storage and thus do neither use the OSs' implementation, nor libuvs' wrappers (as it may be ran in some browser). In this example, using nestdbs' node variant in the browser, with default webpack polyfills and BrowserFS as a node
fs
compatible filesystem adapter implementation, is completely possible, but chokes because of the is windows-check for determining fsync directory capability.Testing for the actual error responses of filesystem operations allows to silently ignore the incapability, based on the actual implementations' capabilities.
What is being changed:
No API changes. No observable behavior changes with default node
fs
module.Changing the original behavior of branching out of
flushToStorage()
... to checks of the fs operation return codes.
When calling
open()
on a directory yields anEISDIR
error, we can safely assume no fsync dir support. Access permissions and rights for the current user should be checked onopen()
and yield anEPERM
error if not sufficient. AnEPERM
error should thus never be returned for a call tofsync()
on fsync dir capable implementations, as permissions have already been checked. It is thus assumed, that aEPERM
(returned on Windows) orEISDIR
error safely indicates a fsync dir incapability and not a permission error.EPERM
as indicator) (platforms as above)byline
test run with windows line endings to validate storage behavior on windowsAdd or update tests, if applicableUpdate documentation, if applicable