Skip to content

Instantly share code, notes, and snippets.

@p
Created January 26, 2012 21:22
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save p/8b8e98d22dd9c9b2ea31 to your computer and use it in GitHub Desktop.
Save p/8b8e98d22dd9c9b2ea31 to your computer and use it in GitHub Desktop.
(15:54:33) bantu: nn-: you fixed some locking problem with flock() we had in 3.0.x earlier, right?
(15:54:48) bantu: have you considered just switching to db locking?
(15:56:14) nn-: it's on the todo list i'm sure
(15:56:49) nn-: i suppose we could do db locking
(15:56:55) nn-: abuse config table for it
(15:57:05) bantu: we do have a lock_db class in 3.1.x
(15:57:09) nn-: do you want it in 3.0?
(15:57:22) bantu: I'm not sure.
(15:57:25) nn-: hmm really, what does that do
(15:57:33) bantu: nn-: uses config table ;-)
(15:58:28) nn-: says here it was done for cron
(15:58:38) bantu: yup, probably
(15:58:49) bantu: but it's reusable
(15:59:26) bantu: You apperently knew that there can be problems with flock (e.g. on NFS).
(15:59:39) bantu: So that's why I am wondering whether you considered using the DB for locking.
(15:59:52) nn-: i think that was the plan long term
(15:59:58) nn-: i'm sure it was written down somewhere
(16:00:04) bantu: oh okay
(16:00:07) bantu: guess that makes sense
(16:00:12) nn-: for queue we're doing it this way because it is currently done that way
(16:00:21) nn-: also, the queue change predates cron
(16:02:05) bantu: well yeah, that might be the case
(16:02:07) nn-: this lock db class looks prettyp fancy
(16:02:25) nn-: if it works we should probably use it :)
(16:02:27) bantu: as far as I remember the changes for flock were quite complicated
(16:02:52) bantu: so we must have considered using db for locking at that time
(16:03:06) nn-: cron ought to lock per task
(16:03:18) nn-: having that many lock files would have been complicated
(16:03:55) nn-: not to mention they don't actually work in all environments
(16:03:56) bantu: Isn't it just one big lock for cron right now? I don't know
(16:04:04) nn-: i hope not
(16:04:26) nn-: * @param string $config_name A config variable to be used for locking
(16:04:43) nn-: ah
(16:04:53) bantu: as for db locking for queue, I'd say 3.1 is fine
(16:05:02) nn-: so we still have my proposal of adding a flag file in 3.0 if we stay with flock
(16:05:28) nn-: hmm
(16:05:32) nn-: phpBB/cron.php:$cron_lock = new phpbb_lock_db('cron_lock', $config, $db);
(16:06:38) bantu: so global locking
(16:06:42) nn-: yep
(16:06:47) nn-: lock db uses __construct
(16:07:01) nn-: does php4 have private and friends?
(16:07:07) bantu: no
(16:07:22) bantu: it does also not have the ocnfig class
(16:07:32) bantu: nn-: is there an actual problem with the queue code in 3.0.
(16:07:39) bantu: it seems to be owrking fine for most people
(16:07:58) nn-: you need to have queue interval set to exceed the interval between message submissions
(16:08:39) nn-: if you have periods when that is not true queued mail will be sent, albeit with a delay
(16:08:57) nn-: on a very active forum with a long-ish queue interval this should break
(16:09:04) nn-: this is all based on reading the code, i haven't tested it
(16:10:43) nn-: the fix for 3.0 should be 10 lines of code or so
(16:11:22) nn-: 1 file existence check, 1 mtime check, 1 fopen/touch and 1 unlink
(16:12:13) nn-: another 5 lines if we want to be careful with the unlinking and try to only unlink the lock file that we created
(16:12:16) nn-: or rather the flag file
(16:17:12) nn-: for 3.1 we should use lock_db for queue
(16:17:34) nn-: this should actually be rather straightforward
(16:19:32) bantu: nn-: sounds good to me
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment