Armadietto is prone to 'Locked !?!?' file-lock errors in real world s… #90
Conversation
…ituations: the timeout of 200ms is insufficient when batch putting or getting. Provide file lock timeout flexibility and make gets non-locking: just cross-checking etag before and after.
|
Please run My tests suggest a lock wait of 30 seconds is optimal, but there may well be scenarios where a different lock wait is appropriate. |
Do you mean 30 milliseconds? |
|
There are two factors affecting how long a request should be allowed to take:
So, we have a lot of head room. My stress tests suggest that 30 seconds is the point of diminishing returns. If other people test different configurations and find different points of diminishing returns, we should use the more conservative value. |
|
Maybe I misunderstand what the lock is supposed to prevent exactly. As a server operator with tens of thousands of users and requests in the hundreds per second, I've never seen a scenario where we needed locking to begin with (with our own setup that doesn't involve Armadietto that is). Is this about folder-listing files perhaps? It seems to me that whatever necessitates that high of a locking period should be an implementation issue, not an inherent race condition issue with the RS protocol or HTTP. |
|
The need for exclusive locking arises from RS needing to recalculate ETags for parent directories on write, up to the scope (top level directory). So, while writing anywhere in a scope, until the ETags for all the parents of the document have been updated, no valid ETags are available for those directories, and no reads or writes can take place. (Writing to a document in a different directory could update some directory ETags, but not the top-level one.) File locks are not great theoretically, but that's what's available for a file system. Other backends might handle that differently - CouchDB is built to efficiently calculate aggregates from documents, so that might be a better back end for RS. Servers serving something other than RS may not need file locks at all. Note that different scopes for the same user are not locked, and other users' storage isn't locked, so the server can serve requests for other scopes or other users at the same time. I'm fine with the lock timeout being a settable parameter, but the default needs to be much higher than the current 200ms. Related to this, "429 Too Many Requests" should be sent when the lock times out. That makes it clear that resolution needs to happen on the front end, not the back end. At present that's mainly so logs are helpful (5xx error codes usually should have a human see what's going on), but remotestorage.js can be updated to understand it. #85 does this. |
|
So that is a detailed "yes" to my question "Is this about folder-listing files perhaps?", correct? At 5apps, we keep metadata in Redis, and we don't even look at the filesystem when delivering listings and ETAGs, so that explains why we don't need any locking for this. Since 30 seconds is an incredibly long interval for real-world use cases (e.g. Litewrite saving documents every few seconds), for the mid- to long term I would suggest looking into changing Armadietto's implementation to solve the problem in a different way. Also, batch uploads are not the same as normal app usage, and personally I would optimize servers for app usage first and foremost, and try to solve batch upload problems in batch upload client code. |
|
Thinking about this more, I actually still don't understand why a lock (of 30 seconds) would be necessary.
This is not really true. You can absolutely read all you want, while writing simultaneously to various directories and files within a folder. The parent folder ETAGs are only used to notify clients of changes within a directory, but they do not indicate what changed exactly. So any change of the parent ETAG just triggers a sync that will check the ETAGs of the children of a folder (via its listing), all the way to wherever changes occurred. The only thing that would fail gracefully is if a client wants to Maybe I'm missing something essential here, or maybe it's not clear to everyone that folder ETAGs aren't really exact versions of folder state? |
It's not clear to me what makes that a long interval. It's not a long time for an HTTP request to fulfill (as noted above). No UI is waiting for the request. There are no other processes waiting on the lock. It doesn't impact other apps or users. Is there a specific scenario where requests taking tens of seconds to complete is distinctly worse than failing some requests unnecessarily? We should definitely work to reduce the length of time that locks are needed. (One straightforward change would be saving a PUT to a temporary file, so the lock is only needed while the file is being moved, which is fast on modern file systems.)
I hope we can do that, but that's a different concern than how long a lock is allowed to exist. We should prioritize a client discovering and reading changes over a client sending new changes, but a shorter lock time doesn't do that - read and write requests are equally likely to be failed. |
Would you mind elaborating why any requests would fail in the first place, in a way that is not graceful, transparent, and easily handled by clients? (With a perfect implementation that is, just for the sake of getting on the same page about the underlying issue). It would be helpful to address my above second reply/question to clarify the exact problem that is being solved here. |
That's the effect of a lock timeout - the FileTree backend throws an error. The request handling code catches the error and fails the request, like other caught errors. Currently, the request-handling code sends a 500 HTTP status for any error, suggesting that the server has a problem. That's not graceful at all. (#85 adds code to throw a distinct error when a file lock fails, and the request-handling code fails the request with a "429 Too Many Requests".) A failed request discards the work done by client and server. That shouldn't be done without gaining some advantage. It sounds like you are starting from the premise that failed requests are not a significant problem. I hope remotestorage.js handles failed writes well. My tests suggest failed writes create unnecessary conflicts, which in general need to be resolved by the user. If your tests show it does retry failed writes, it very likely does. However, I don't think we should base decisions off that until we have some hard evidence that failed writes don't create unnecessary conflicts. Correctness is more important than efficiency. The spec does not address how failed writes are resolved, so far as I can tell, nor can I find a discussion of this in the forms. I am starting from the premise that failed requests are a significant problem. I have observed them happening at request rates of 10 requests/second, which is very disappointing on modern hardware. @JakubNer has observed them as well. Increasing the lock timeout to 30 seconds increases the rate at requests can be successfully handled by over 30 times. (If people prefer 9 seconds, to keep it under the usual 10 second ping, that's also very good at preventing failed requests.) It sounds like you are also starting from the premise that long lock timeouts are bad. I do not see any evidence that long lock timeouts are a problem for our architecture, nor am I aware of any scenarios where we expect them to be a problem. Separately, the more that the server can handle other requests, while a lock is in place, the less long locks are a problem. |
|
The issue documented by remotestorage/remotestorage.js#1259 is not handled in a graceful or transparent manner, and is nontrivial for clients to work around. Given that such a small deviation from the happy path is handled so poorly, I don't know that we can depend on remotestorage.js handling edge cases well, without specific evidence. |
|
Hi, just catching up. Right, the specified time is the amount of time we'll block the requesting thread until the If I have multiple nodes and request 100 files at once, it may very well be on the order of seconds between the last file was getting the Will run The 429 idea is good but I don't believe 429s get any special handling in remotestorage.js from what I recall. The retries in remotestorage.js do work quite well, just takes 10s to loop around. But eventually it syncs. |
I briefly thought about that. I do have redis in my cluster and put some dependencies on redis in already in another PR. But I figured there was simplicity in using the filesystem in the way the original authors of armadietto wrote it. Or rather I didn't want to make my redis persistent nor start dealing with transactions or atomic GETSETs :). Let's see how far we can push this |
No, not at all. I'm questioning why there have to be errors at all, and I'm saying there shouldn't be any, but only non-error HTTP statuses, like 412 for example. My actual questions and explanations have not been addressed at all so far. |
Armadietto logs 5xx errors as critical, while most 4xx errors are logged as warning: Depending on the machine configuration, critical errors may trigger an immediate message to the operators. |
When a write request for a document is quickly followed by a status ping (typically from a second device), the lock architecture ensures that the status ping correctly informs the second device that it has stale information. The second device can then request the changed document and immediately display it to the user. If you are suggesting we eliminate locks entirely, then the status ping would return stale information. At the next sync interval, the next ping would inform the second device that there are changes, and the second device would download them then. Is that what you mean by 'failing gracefully'? The lock-less architecture delivers updates a half-sync-interval later, on average - 5 seconds for the default sync interval. In many scenarios, that is fine. However, in the scenario where a user is interacting with their phone and switches to a computer, any changes made on the phone will not show up on the computer for an extra 5 seconds. That's five extra seconds that the user is waiting on the software. That plus the average 5 second time for changes to propagate will, half the time, be longer than the 10 second limit where the user's attention is lost That's five extra seconds (even for mistakes or trivial space changes) where the user could type something on the computer, and cause a conflict. For the scenario where two users are sharing an account (say, two students working on a paper together, or a couple making a shopping list), conflicts are even more likely, since both can be typing on different devices at the same time. Conflicts are a terrible UX - in general, the user must presented two versions of the document and choose one, discarding part of the changes they just made. Programmers understand the need to merge changes (but hate to actually do it). Ordinary users don't really understand this kind of technical issue - they want software to "just work". To ordinary users, conflicts appear to be the software malfunctioning. It can be argued that this is a tradeoff between single-user performance and overall throughput. If you want to submit a PR that adds a configuration item to disable locks, I'm fine with that. But the default should be locks. Correct behavior first, performance later. |
|
Interesting points. However, quite different from the earlier conversation!
That is how sync works with rs.js, yes. I'm not aware anyone ever did more status pings than the normal sync interval (which you can configure, both globally and on the fly). It certainly wouldn't be advisable in most scenarios, since hammering servers with requests for no reason is inefficient both for the server and the client.
Yes, I've never seen this not be fine actually. And again, you can change the sync interval in rs.js, if your app or use case requires it.
There is no need to wait for the software in most cases. And it entirely depends on how you design your software. Also, when you open an app it usually syncs immediately, not after 5 or 10 seconds, so depending on your connectivity, you should see new documents or changes come in sooner than that.
First of all, you're not going to get rid of a delay for updating data from the remote with any solution that uses a server to sync data. Secondly, if you knowingly switch devices and you want to edit the exact same document on the other one, then in my experience you would wait a couple of seconds, until you see the change having been synced. I have not seen this as a problem before, and there is no logical way of solving this, other than doing background sync at high intervals on all devices that might potentially be switched to. Furthermore, RS is explicitly a protocol for personal user data, and collaborative apps should do p2p data sync and only then sync data to the respective user's remote storage. And even then, I know Grouptabs e.g. worked just fine when sharing one account with many people (we've used it for many years), and conflicts were never an issue. So again, it always depends on your data model and also your conflict resolution mechanisms for your specific use case.
It could just as well be argued that single-user performance is actually worse with locks, because a user may close an app too early and not have the data synced to the remote in time, before opening an app on another device. This is not a clear-cut case of improving performance for all or most use cases in my opinion. |
|
Functionally, from the protocol perspective, the I'm not as familiar with the protocol as you guys. I just kept the implementation at functional parity, only improved performance. But I'd love to constrain the atomicity to individual files if the folder-level etags can be eventually consistent w.r.t. adherence to protocol. |
That is what I'm trying to get to the bottom of. It doesn't seem to be necessary for the implementation we used at 5apps for a rather long time now, so I was just trying to help here by finding out why it is necessary in Armadietto. |
I'm assuming the app is already running on both devices, thus their syncs are out of phase by 0-10 seconds (for the standard 10 second sync interval).
I'm not saying there's any polling architecture with no delay. I'm explaining how the lockless architecture doubles the average machine-to-machine delay. The increase in user-perceived delay is even greater - if you assume a user takes two seconds to switch devices, the user-perceived delay for the lock architecture averages 5 - 2 = 3 seconds. The user-perceived delay for the lockless architecture is 10 - 2 = 8 seconds.
The solution for that is Background Sync which handles many more scenarios besides the scenarios we've been discussing.
For any document that is being written, the lock must cover that document and all its parent directories up to the root directory. Thus, if two different documents are being written, their locks will overlap at the root directory and possibly other directories. For read operations, only the document itself must be locked against writes. Multiple reads of a document or directory can occur simultaneously. The simplistic lock system currently implemented could and should be upgraded to this multiple-lock system. |
You still haven't explained why that is the case, even though everything I wrote was about trying to find out why you think that that's the case. I know it may be obvious to you, even if it isn't for the rest of us. But if you really think there's a good reason why folders need locks, instead of just single files, then it would really help us if you could explain it, so we're all on the same page. Everything else you're stating is only about performance optimizations for the edge case of users having an app open on multiple devices at the same time, while in most cases (at least in my personal daily use of smartphones since the iPhone 1), you usually open an app when you switch, instead of having it open already. |
As noted above, as soon as the server has a complete write request, the ETags for all the directories above the document are stale. Returning stale ETags means the client won't request the new document until the next regularly-scheduled sync. Clients with stale documents can produce conflicts that could have been avoided. These conflicts can occur in ordinary usage. Conflicts in general require the user to manually resolve. Asking ordinary users to manually resolve a conflict will be perceived as the system failing to store their data reliably. Failing to reliably store a user's data will drive them away from RS, probably permanently.
What you're calling performance optimization and edge cases are the difference between "it works for me, most of the time" and "it works for everyone, all of the time". We are competing against disk drives, which have an astonishingly low rate of data loss, and the problem we're tackling is inherently harder than that of disk drives. |
|
Again, what you're stating there is not a real-world problem with most use cases and apps, since the remote storage is merely a sync mirror of the offline-first local storage, so a client does not by itself have any problems whatsoever with stale folder ETAGs that I'm aware of or that you explained. Switching apps while the app is open on another device is absolutely an edge case, and even in that case it's entirely up to the use case, app, and data layout for how much of a problem you would have with conflicts to begin with, or resolving them if they happen. As you stated yourself, the only way to properly solve that case is with background sync, not with folder-level locking. And even then, it can still happen and you also wouldn't want all your apps do sync requests all the time, because it both eats up battery as well as strains the resources of the storage server. But more importantly, what you still haven't addressed is this:
I think there are more cases where not being able to write multiple documents to a folder at once before closing an app is much more of a problem when switching devices than conflicts from stale folder ETAGs (which would only occur if index files are written so slowly that even a few seconds aren't enough for it to be done before the next sync request). Another worse case for user experience may be batch imports, which would be much slower than they have to be when you cannot PUT multiple documents at once. Think importing 1000 bookmarks for example. |
|
It appears we are focusing on different use cases. If you want to add an option to disable locks, I won't oppose it. I cannot agree that we should remove locks entirely. |
|
If you think that, then please explain to me which cases you think I'm focusing on, that you think aren't important or as common as yours. ("Yours" being the only one that global locking seems to be a benefit for, while appearing to be a drawback for all others). |
That's a false dichotomy by the way. I never proposed to remove file-level locks, but only questioned why Armadietto is the only server implementation that needs global locks for all PUTs. |
|
So, I just tried to sync ~2000 bookmarks after a fresh login on localhost with Webmarks, and I couldn't even get it to finish within 10 minutes. In fact, I had to give up eventually, because it was so slow. It would throw a couple of 500s about the lockfile every 10 requests or so. The current state is basically a broken server implementation for a normal use case of initial sync when connecting your storage to an app with enough existing data in it. I also just re-read the whole conversation here, and still haven't found a good argument for while there needs to be global locking in the first place. |
|
Development of armadietto appears to have stalled because of this issue. We don't have to have a consensus on the long term direction to resolve this PR. @raucao, can you suggest a way forward on this issue? In particular, is your objection to this PR that
If it's 2., this PR does not lock when we aren't locking already, and does not increase the length of time locks are held. It makes no changes to the thread which obtains and releases the file lock. It only affects whether requests in other threads succeed or fail. In other words, it decreases the impact of file locking. |
|
I have no objection to this PR, and never voiced one. I was trying to find out why Armadietto had global locks in the first place, because I never knew it before this PR. Then I also ran into not being able to sync my bookmarks in testing, because of the locks, which makes this a critical bug in my opinion. However, that would seem to be unrelated to this pull request, and we should probably have switched to a dedicated issue about locking improvements at some point. In summary of the above discussion, I would suggest the following:
|
| @@ -16,12 +16,14 @@ const writeFile = promisify(fs.writeFile); | |||
| class FileTree { | |||
| constructor (options) { | |||
| this._dir = path.resolve(options.path); | |||
| this._timeout = options.lock_timeout_ms ?? 200; | |||
There was a problem hiding this comment.
This is the length of time another thread will wait for the lock to be released and thus, an upper limit on how long an HTTP request can take before it gives up because of the lock.
Firefox will wait 10 minutes for an HTTP request to succeed, other browsers wait longer, giving us plenty of headroom there.
Defaults for the remoteStorage library are:
requestTimeout: 30000,
syncInterval: 10000
My stress tests (see #85) suggest 30 seconds is the point of diminishing returns.
I recommend we set the default value of this._timeout to 30_000 (30 seconds)
| async get (username, pathname, versions, head = false) { | ||
| const startTime = parseInt(Date.now()); |
There was a problem hiding this comment.
Date.now() returns "the number of milliseconds elapsed since the epoch" so parseInt is unnecessary here and line 181
|
overhide#1 makes the changes I requested above. |
…see #90) (#112) * Armadietto is prone to 'Locked !?!?' file-lock errors in real world situations: the timeout of 200ms is insufficient when batch putting or getting. Provide file lock timeout flexibility and make gets non-locking: just cross-checking etag before and after. * Skip node 12. * Add hosted load test link. * Sets default lock timeout to 30 seconds --------- Co-authored-by: Jakub Ner <[email protected]>
|
Closed in favor of #112 |
Armadietto is prone to 'Locked !?!?' file-lock errors in real world situations: the timeout of 200ms is insufficient when batch putting or getting.
[1] Added config points:
[2] Added a load test HTML file in
/example/load.html: also run it withnpm run serve.[3] Made the file_tree implementation not use a lock on
get-- gets are optimistic. Made it check the etag before and aftergetinstead. Onetagfailure we respect thelockfiletimeouts for retries until we give up.