Re: cvsup broken on amd64?

On Mon, Sep 19, 2011 at 8:26 AM, Adrian Chadd <adrian_at_freebsd.org> wrote:
> 2011/9/19 Alexander Zagrebin <alexz_at_visp.ru>:
>
>> I've tried this patch. Now csup "hangs" before handling fixups.
>> So there is no message "Applying fixups..." at all.
>
> Wow. Hm. Where's the author when one needs them..

Well that's quite a strange coincidence. I haven't read current_at_ in
ages and now I just stumble upon this. :-)

It's been a long time since I've been looking at this code but the
patch from the original PR seems /nearly/ correct, while the one from
adrian_at_ is definitely incorrect. But to be honest, this part of the
CVSup protocol is a lot more twisted than it would seem, plus a
comment of mine was definitely misleading (it should say that the
detailer thread will hang, not the lister one). Here are some
explanations that should help clarify things.

When the updater thread encountered a problem in updating a file (MD5
checksum doesn't match), it will send a fixup "request" to the
detailer thread. The detailer thread then sends this request to the
CVSup server, which, finally, sends the whole file for the _updater_
thread to receive. So what's happening at this position in the code is
that the updater thread finished processing all the "normal" updates,
and thus knows there won't be any more fixup requests (fixups
themselves can't generate other fixups). This is why it closes the
writing end of the fixups queue, to wake up the detailer thread which
will notice the closed state and the absence of fixups in the queue,
and will thus terminate. So we _have_ to call fixups_close() here, or
the detailer thread will block indefinitely in fixups_get() when an
error happened.

Knowing all that, what's happening seems quite clear. If
fixups_close() is called while there was still fixup requests pending,
those should be processed by the detailer thread before it returns.
Subsequent fixups_get() call should continue returning pending items,
until there are no more entries in the queue _and_ the queue is
closed.

So, line 144 in fixups.c (in fixups_get()):

        if (f->closed) {

should actually be:

        if (f->closed && f->size == 0) {

The fact that this could even happen smells a bit weird to me though;
it means the detailer thread didn't get a chance to run between some
item was added to the queue (fixups_put() signals the detailer thread
via pthread_cond_signal()), and that it only runs now that the updater
queue has been closed. I wouldn't go as far as saying this is a bug,
but is it even valid for the pthread library to "coalesce" two
pthread_cond_signal() calls into one when the thread didn't get to run
since the first call was made? If so, then the above fix is perfectly
valid. If not, there is a deeper bug in the threading library, or in
the CVS mode code which I didn't write (but that seems far-fetched).

It would be great to fix my misleading comment too by the way :-)

I hope this helps.

Cheers,
Maxime