Stephen Finucane <step...@that.guru> writes: > On Sun, 2018-02-25 at 01:50 +1100, Daniel Axtens wrote: >> Thomas Petazzoni reported that Patchwork would occasionally lose >> Buildroot email. Andrew - having talked to jk and sfr - suggested that >> this may be race-condition related. >> >> I investigated and found some bugs. I first had to develop some tools. >> Along the way I found other unrelated bugs too. >> >> Patches 1-4 are tooling - ways to do parallel parsing of messages and >> get and compare the output. (Patch 1 fixes an issue I found when >> running the tool from patch 2) >> >> Patch 5 is an unrelated fix that came up along the way and >> demonstrates that humans remain the best fuzzers, and that Python's >> email module is still adorably* quirky. >> >> Patch 6 is a bug that came up very quickly in testing but is unlikely >> to be the actual bug Buildroot is hitting, as it can only occur the >> first time an email address is seen. >> >> Patch 7 is a related tidy-up/optimisation. >> >> Patch 8 fixes up a MySQL-only bug, but also adds some robustness. >> >> I think patch 9 closes the most likely issue for Buildroot patches. >> >> Pending review, patches 5, 6, 8 and 9 should go to stable. >> >> V2: Address review comments from Andrew, see patches. >> Added the new python script to the automatic pep8 testing. >> Minor improvement to parallel parse script to allow different >> pythons to be used. > > Well, this is all rather unfortunate :( When I was developing this, I > was testing using 'parsearchive' with a single archive, which naturally > meant everything was running in series. It seems the use of an MTA > means the 'parsemail' script, which 'parsearchive' harnesses, gets > called multiple times here and highlights all these races.
Likewise, and yes, I think that's how we get here. > I've reviewed most of the patches (bar one, which I'm still working on) > at this point, and think most of these should be merged. However, I've > noted that some of the patches leave FIXMEs in place. I'm pretty sure > all of these occur where we apply a "lookup now and save later _if_ > these preconditions are met" pattern, which we do for things like > series references, authors, etc. This isn't going to be easy to fix, > IMO. > > That brings me to the real question here: what do we need to do to > ultimately fix this? Far as I can see, there are two options: > > * Save stuff unconditionally. If we're able to find an email, save > that even if it's not a patch/cover letter/comment. If we find an > author that we haven't seen before, save that too even if they > haven't touched a patch, cover letter or comment. This greatly > simplifies things, but would result in a more congested database and > API, and would require some background work (saving non-patch/cover > letter emails). I don't think we need to do this - and I'm not sure that the problem is the lookup-now-save-later pattern, I think it's just our implementation. The big thing is series references, and I think that (apart from unthreaded series and possibly deeply threaded series?) we can solve that pretty easily by holding the 'head email' (patch 0 or patch 1) in the SeriesReference. But I will think about it more; I don't have certain answers just yet. > * Use locks. I recall a patch from Damien Lespiau some years ago > around making this a parallel process [1]. Couldn't we just do this > on the critical sections of code (anything that involves DB > accesses, I guess)? This would require minimal changes but could > have scalability issues. So I have Feelings about introducing locking mechanisms. In particular, looking at [1], I worry we're going to land in the territory of bizzare and irritating issues that robust futexes / pthread mutexes deal with. In short, a futex is a fast userspace mutex device facilitated by the kernel. It's used to implement higher-level locks/mutexes/semaphores. It uses shared memory - basically it twiddles bits in memory to use as a lock. Now, consider 2 processes, A and B are using shared memory and futexes. A takes the futex, and B sits waiting for the futex so it can proceed. While holding the futex, A segfaults. However, A is still holding the lock, and cannot clear it, so B is now deadlocked. It's easy to go "oh, we should catch segfaults", and with the right handler functions you can. So let's say A does that. Now if it segfaults, the handler unlocks and B continues. This is all well and good, but now consider the situation where an impatient sysadmin (or the kernel) SIGKILLs A. In this case no handler can help you: B will always deadlock. Robust futexes solve this problem with kernel magic - that's what makes them "robust". (Basically, the kernel deals with the case of the process going away with the lock held.) So, in Damien's case: - we could deadlock if there is a Python exception - so we catch this with a finally: and unlock - but we're still stuffed if the python process is killed in a way that stops Python from running the finally block So we will have introduced a new locking mechanism with new, fragile, tricky to debug failure modes*. There is no mechanism we could conceivably hook into to become truly "robust". Now, Damien's code does try: the underlying implementaion in https://lists.ozlabs.org/pipermail/patchwork/2015-October/001957.html makes the lockfile (/tmp/whatever) as a symlink to "hostname:pid" as this is atomic. Good! But, to avoid the robustness issue, if you try to take the lock and it's owned by a process with a PID that does not exist, it guesses that the process has died and steals the lock. This, doesn't work if any of the following apply: - you're in different PID namespaces but the same hostname namespace (e.g docker --net=none). - there have been enough processes that the PID counter has rolled over and the PID has been reused. - the code tests for the existence of a process by sending signal 0 to the PID and seeing what happens. It's very easy to imagine a paranoid sysadmin from using seccomp to block parsemail from sending a signal, as there's no obvious reason why it should do so. (And I checked strace, it doesn't appear to do so at the moment.) To add insult to injury, we've added this concurrency control mechanism to solve a problem in how we use an underlying system that has (literally!!) decades of work in correct concurrent behaviour. I would rather not do this. Regards, Daniel * More interesting failure modes: - Each parsemail process runs in a docker container with a unique /tmp - PW runs on a RO filesystem - PW runs distributed across machines - have to share /tmp (and w/ something that supports atomic symlinks e.g. NFS) - can another unrelated process delete the lock file? > > Thoughts? > > [1] http://patchwork.ozlabs.org/patch/532930/ _______________________________________________ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
