Currently there is no locking when a source is being loaded for importing, clearing or expiring.
Especially when importing, this can lead to duplicates or other kinds of unexpected behavior. #600584: Use Batch API will make it only likelier that such unexpected behavior occurs (see how caching is handled between batch stages).
We need a locking mechanism that makes sure that only one process updates/clears/expires a source.
$claimed = FeedsSource::claim($id, $feed_nid);
FeedsSource::release($id, $feed_nid);
| Comment | File | Size | Author |
|---|---|---|---|
| #3 | 640508-3_locking.patch | 3.16 KB | alex_b |
| #2 | feeds_lock_schema.txt | 1013 bytes | alex_b |
| #1 | 640508-1_locking.patch | 4.55 KB | alex_b |
Comments
Comment #1
alex_b commentedAdds FeedsLock class and corresponding feeds_lock() and feeds_expire() helpers.
I am torn between 2 approaches:
- To arbitrate between cron and batch updates we would need a multi-page callback locking system. It would make sure that no two multi page load processes get into each other's business until they are done. But such a locking system requires more infrastructure: a multi page process would acquire a lock token and every page load would verify that that lock token is still valid and at the end of the multi page process this token needs to be released. Every user of the locking API would need a place to store and recover that token between page loads. A UI for breaking such locks would have to be built.
- We could go the easy route and really only make sure that two processes can't execute the same operation on the same resource at exactly the same time. Here a simple in-page locking system like Drupal core's would be enough. The downside is that interaction between processes aren't clear at all.
Comment #2
alex_b commentedHere's the schema for the above patch.
Comment #3
alex_b commentedThis patch implements #1's "easy route" by not worrying about locking across page requests but focussing on avoiding the same operation on a single source/importer at the same time.
This patch uses Drupal's locking API to:
- lock per "import" on a particular source of a particular importer.
- lock per "clear" on a particular source of a particular importer.
- lock per "expire" on a particular importer.
This patch should take care of situations where imports of the same source by multiple concurrent processes lead to duplicates as reported in #704272: Duplicate node entry when cron runs.
There are still possible interactions between the operations import, clear and expire. However, we can only fully address them if we disallowed expiry on an importer level. Otherwise we would have to block on an importer level which is simply not practical.
Comment #4
ekes commentedIt's working certainly working.
But it is a bit blunt, setting one timeout, 60 seconds, for the full processing of a feed - including as many items as are set per batch. In my edge case (enclosure download) I don't even know if the enclosures will be bytes or gigabytes - think of the feeds people make primarily for consumption with the miroplayer. I'm presently looking at at least touching the lock at the item process level, so the time gets updated on each item - sound sensible?
Comment #5
alex_b commentedekes: I'd rather make the expiry seconds configurable as a variable.
Comment #6
ekes commentedI'd assumed the configurable variable bit. But it was still difficult in my somewhat marginal situation to guess how long x items should take maximum without setting a rather large time. Single items (a huge video file included) can take an hour or so (but could take no time at all if only a tiny file). So even with a setting of only 5 items to be downloaded each time it makes the timeout 5 times larger than needed and thats hours! Setting a maximum per item was easier.
I've patched it to do this and I'd suggest it was worth it as it's a simple addition, just touching the lock each item, except it falls foul of #838976: lock_acquire() breaks if new expiration is the same as old one
Comment #7
alex_b commentedekes: you are actually making the case for a multi page load lock like I outlined as an option in #1...
Such a multi page lock could be much easier to do once we have more solid batching #744660: Expand batch support to fetchers and parsers.
Comment #8
Docc commentedIm using the patch. But still getting duplicates. The duplicates show up with a feed_nid value of 0
:/
Comment #9
Docc commentedright, found my problem. Got duplicates with feeds_nid 0.
This was due changing the feed from manual (stand-alone-form) to being attached to a content type.
Though in the feeds_sources the reference did not get deleted to the stand-alone-form. And so content got imported twice. I manually deleted the row from feeds_sources
Comment #10
alex_b commented#9: I opened a new issue for your report: #906654: Fix phantom subscriptions
Comment #11
Docc commentedyeah tnx, sorry bout that. Initially thought it was the same issue.
Comment #12
meatbag commentedI still get duplicates after applying this patch. Just not as frequent as before.
And i can't get anything useful from the log messages.
A more verbose logging system is needed to troubleshoot these kinds of problems.
Comment #13
alex_b commentedmeatbag: agreed #606612: More detailed log
Comment #14
alex_b commentedTo summarize this issue: as ekes reported in #6, lock_acquire() won't go all the way.
- We need to implement source level locking at least for import() and clear().
- We need to add a UI component that shows import or clear status on a feeds 'import' or 'delete items' tab and denies manual importing or deleting during cron imports or other users manually importing/deleting at the same time. There should be a way of breaking that lock.
- A lock for expire() may wait if it proves to be a headache. It is not as critical, only runs once an hour, can't be triggered from UI.
Comment #15
kenorb commentedClosed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.