[RubenVerborgh]

Closes #1106.

This does not address other HTTP methods.

[timbl]

If this is using file locking, then it will need to have the lock also for PUT requests and GET requests - as a GET request could come in the middle of the patch and get a half-written file.

[RubenVerborgh]

Agreed; as stated in the issue text, other methods are not done.

[kjetilk]

@megoth tested this module in a benchmark, and couldn't get it to perform reliably, so we need to be careful.

[RubenVerborgh]

What benchmark, what does "reliably" mean, and compared to what?

If we are testing 100s of PATCH requests to the same file, then yes, you will see degraded performance compared to the original code. That's because the original code will happily handle two PATCH requests simultaneously and mess up the file.

The good test is 100s of PATCH requests to different files.

[kjetilk]

I'll let @megoth fill in the details, his tests are here:
https://github.com/megoth/lock-test
but the problem appeared to be that it did in fact allow overwriting the file in some cases even when the file was locked.

[RubenVerborgh]

Ah the proper-lockfile module, not this PR. That seems very weird, as the library is heavily tested and widely used. Happy to use any other locking library.

[kjetilk]

Yeah. Pretty weird.

I might add that we reviewed like 25 different modules on npm. Most relied on a db to manage locks, mostly redis, which we found to be a too big dep to introduce. Many were mutex modules that locked the whole process rather than a file, some, like proper-lockfile and lockfile did not work as advertised...

We also went through some options with @justinwb on Friday.

Which left us with the possibility that we need to do something POSIX-specific... Anyway, you might want to talk with @megoth about it on Monday.

[RubenVerborgh]

some, like proper-lockfile and lockfile did not work as advertised...

I'm inclined to say this is likely a test error rather than a library error, given the popularity of those libraries.

[kjetilk]

Perhaps, but I wouldn't bet on it before we have confirmed it for ourselves.

[megoth]

Are we certain proper-lockfile works as intended? Is it possible to write a test for this?

Given that this works as intended, the code itself looks good (hence my Approve to this PR).

I did try to make a testsuite for locking, with the intention of being able to benchmark various solutions (https://github.com/megoth/lock-test), but I wasn't successful =/

[megoth]

Ok, so with #1118 rebased on this branch, and with a minor tweak to https://github.com/Inrupt-inc/generator-solid-react/tree/release/profile that enables me to spam the server with PATCH calls my findings are (drumroll) that this PR seems to be work!!! 🎉

Spamming the server also does not seem to slow down the handling of other resources noteworthy, which is also as expected with this solution.

Granted, this test is not 100% proof that this prevents data from concurrent writing , but I'm adding my approval fully behind this PR at least 😸

[RubenVerborgh]

Great! Still need to address @timbl's concern of also locking other methods.

[RubenVerborgh]

Locking added on GET/PUT/DELETE; all good to go now.

[kjetilk]

LRGTM. The old code was rather hard to read to me, so not only seems it do what it should in terms of locking, but I think I understand it a bit better, but more reviews would certainly be appreciated.

[kjetilk]

@timbl are you good now?