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.

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

Support fs.flockSync and fs.funlockSync #49256

Closed
fabiospampinato opened this issue Aug 20, 2023 · 10 comments
Closed

Support fs.flockSync and fs.funlockSync #49256

fabiospampinato opened this issue Aug 20, 2023 · 10 comments
Labels
feature request Issues that request new features to be added to Node.js. fs Issues and PRs related to the fs subsystem / file system.

Comments

@fabiospampinato
Copy link

fabiospampinato commented Aug 20, 2023

What is the problem this feature will solve?

These APIs are needed for important functionality, like implementing a virtual file-system layer for sqlite: example.

Without them something like x/sqlite can't quite be cleanly ported from Deno.

What is the feature you are proposing to solve the problem?

I propose adding:

We should probably close the gap with Deno here.

What alternatives have you considered?

I haven't found a nice one yet, I guess I could:

  • Just not implement support for locking, which would make sqlite unsafe to use in some case.
  • Use native bindings to sqlite, but native bindings are a pain to use.
  • Spawn native sqlite binaries directly, but those are also kind of a pain to use, especially under macOS with notarization, and they are slower than the wasm approach.
  • Using some third-party module that provides this feature, but they all require native modules, and not requiring any of them is a goal here for me.
  • Switching to Deno I guess, which is not really practical for me.

Maybe the only somewhat reasonable alternative would be to manually implement some form of locking in userland, which would still make it unsafe to open the same database with other sqlite builds though, which is not ideal.

@fabiospampinato fabiospampinato added the feature request Issues that request new features to be added to Node.js. label Aug 20, 2023
@mscdex
Copy link
Contributor

mscdex commented Aug 20, 2023

This is the wrong repo for that, a cross-platform flock() would need to be added to libuv first.

@benjamingr
Copy link
Member

This is the wrong repo for that, a cross-platform flock() would need to be added to libuv first.

And the fact Deno is broken here on the most popular operating system isn't really super confidence inspiring.

Here is the repo btw https://github.com/libuv/libuv

@benjamingr
Copy link
Member

As for the actual use case and not feature request you have several options:


But I think asking for file locking in libuv is reasonable,

@bnoordhuis
Copy link
Member

It's been asked before in libuv and the answer is no because cross-platform file locking is broken beyond repair or redemption (as deno found out): libuv/libuv#657 (comment)

I'll go ahead and close this because this is one of those things that's simply not happening, sorry.

@bnoordhuis bnoordhuis closed this as not planned Won't fix, can't repro, duplicate, stale Aug 20, 2023
@bnoordhuis bnoordhuis added the fs Issues and PRs related to the fs subsystem / file system. label Aug 20, 2023
@fabiospampinato
Copy link
Author

This is the wrong repo for that, a cross-platform flock() would need to be added to libuv first.

That's an implementation detail of Node, this is the right repo for proposing a change to Node's public API. On the contrary libuv is obviously the wrong repo for proposing adding fs.flockSync to Node, because even if they added locking APIs that would not change anything by itself for Node.

Use the native bindings which honestly really isn't a pain

Well, if using native bindings for a user are a pain for that user they just are a pain for them, you can't just decree that they are not painful in general.

Maybe more interestingly: try to think of all the effort necessary to build native bindings to a version of sqlite compiled with custom flags that works across platforms in Electron... That's obviously way more complicated than compiling sqlite to wasm and just using it.

Use wasm bindings like tndrle/node-sqlite3-wasm which take the pain of using a native binding if such pain exists.

You are totally missing the point here, sqlite needs to be able to lock a file in order to not corrupt the database when accessing it via two different instances of sqlite. That module can't be working correctly precisely for the issue I'm mentioning, it can't lock files, because Node doesn't provide APIs for that, so it can't prevent corruption, so it's not a viable alternative, obviously.

Use a library like proper-lockfle though it seems like this use case isn't popular enough for that sort of thing to be popular. It looks there are some more popular alternatives though like npmjs.com/package/@verdaccio/file-locking - I'm not sure if they're good.

That's not how this works at all though. You need cooperation between different sqlite instances, you can't just make up whatever file locking convention you please, sqlite by default uses advisory locking, if a database is opened by an instance of sqlite that uses some custom locking logic, and by a normal version of sqlite that instead uses advisory locking they would just not be able to see each other's locks, so you'll get corruption just the same.

It's been asked before in libuv and the answer is no because cross-platform file locking is broken beyond repair or redemption (as deno found out): libuv/libuv#657 (comment)

Node has had, and probably still has, APIs that don't work across all supported OS', so I don't see why providing an API that only works on macos/linux would be deal breaker here.

Besides if providing locking for Windows is really hopeless, then libraries that ship native bindings to sqlite like better-sqlite3 can't possibly be working correctly either, so a wasm port of sqlite built on linux/macos-only locking APIs would not really lose any feature compared to that, which would still make it pretty compelling.


Can we re-open this, since there's still value to providing linux/macos-only locking APIs, even if libuv doesn't want to add provide them?

@mscdex
Copy link
Contributor

mscdex commented Aug 20, 2023

Why not just write your own bindings for flock() (or perhaps reuse an addon from npm that exposes such functionality)? That would work whether your copy of sqlite is custom built or not.

@fabiospampinato
Copy link
Author

It's an option I'm considering, but it would go against one of my objectives, which is not relying on any native modules, which are a pain to work with in general, for me and my use case.

@benjamingr
Copy link
Member

https://github.com/yodaos-project/node-flock looks pretty ok if not entirely up to date?

@fabiospampinato
Copy link
Author

yodaos-project/node-flock looks pretty ok if not entirely up to date?

Maybe it's an OK native module, I can't say if the implementation is correct nor if anybody is actually relying on it (pretty flat ~2 downloads/week on npm).

Like all native modules it is a pain in the ass to use for me and for my use case though, so I'd rather not go that direction.

@bnoordhuis
Copy link
Member

Node has had, and probably still has, APIs that don't work across all supported OS

Very few, almost none. The guiding principle since well before 1.0 has been that if it can't be implemented faithfully on all supported platforms, it isn't implemented at all, with very few exceptions. For libuv the bar is even higher.

I'm not going to rehash all the problems with file locking, just suffice to say that it's broken in different ways on different platforms. If you want file locking, cool, but you'll have to get it through a native module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

No branches or pull requests

4 participants