Solaris cloning woes partly diagnosed

This is just an interim report, but people might have heard that OpenSolaris team are in the process of choosing a free DSCM and we are one of the candidates. They initially wanted to try 1.2.4 but had trouble using it for local cloning, and the evaluation is being done with 1.2.2.

I was on #git tonight with Oejet, and managed to reproduce this problem. The local clone problem seems to disappear if we disable the progress bar in pack-objects.

We do two funky things when we have progress bar. We play games with timer signal (setitimer(ITIMER_REAL) and signal(SIGALRM)), and we spit out messages to stderr.

It's too late tonight for me to continue digging this, but if somebody with access to a Solaris box is so inclined, I'd really appreciate help on this one.

On Sun, 2 Apr 2006, Junio C Hamano wrote:

I'd be willing to bet that it's the fact that we take signals.

Suddenly, some system calls will either return -1/EINTR, or they'll return partial reads or writes.

We should be pretty good at handling that, but maybe some place forgets.

One thing to do might be to make the itimer use a much higher frequency, to trigger the problem more easily.

We do, for example, expect that regular file writing not do that. At least "write_sha1_from_fd()" will just do a "write()" without testing the error return, which is bad (it would silently create a truncated object if the /tmp filesystem filled up). If somebody has their filesystem over NFS mounted interruptible, partial writes could also happen.

Ho humm.

		Linus

And Linus Torvalds writes:
 - 
 - I'd be willing to bet that it's the fact that we take signals.

Unfortunately, I'm too busy to check into this, but I've run into similar problems in the past. Just takes a busy file server.

 - We do, for example, expect that regular file writing not do that. At least 
 - "write_sha1_from_fd()" will just do a "write()" without testing the error 
 - return, [...]

There is an xwrite in git-compat-util.h...

Jason

On Sun, 2 Apr 2006, Jason Riedy wrote:

Well, git itself is actually fairly good about these things. Right now I'm seriously suspecting Solaris stdio as being just horribly impolite.

git tends to not just use xwrite() in most places, but check the return value for partial sizes etc. I tried to grep for places where we were lazy, and there really seems to be just a very small handful, and they shouldn't impact this case at all (you have to have a seriously broken setup for them to matter, but we should fix them nonetheless.

		Linus

On Sun, 2 Apr 2006, Linus Torvalds wrote:

Hmm. If I read the IRC logs right, the bad pack is still a _valid_ pack, and passes git-verify-pack with flying colors.

That certainly implies that we had no problems with write-out: not only must the SHA1 of the resulting file match itself, but it must match the index too, and the number of objects there must match the index.

So the only way I see the pack being bad (if it does indeed pass git-verify-pack) is if the object list we generated was bad.

However, "-q" only affects git-pack-file itself, not the generation of the list. Which would imply that we have trouble _reading_ the list as it comes in through a pipe. Which is just insane, because we use just a bog-standard "fgets(... stdin)" for that. And no _way_ can stdio have problems with a few SIGALRM's, that would break a lot of other problems.

But Oeje1 seems to be saying (in http://pastebin.com/635566):

	git rev-list --objects --all | git pack-objects pack
	Generating pack...
	Done counting 15 objects.
	Deltifying 15 objects.
	 100% (15/15) done
	Writing 15 objects.
	 100% (15/15) done
	806439fdfa5e9990b03f9301bd68e243795fff50

where the result _should_ be 16385 objects, not 15.

And the thing is, the _only_ thing we do there is that

	while (fgets(line, sizeof(line), stdin) != NULL) {
		...
		add_object_entry(sha1, name_hash(NULL, line+41), 0);

so it really really looks like fgets() would have problems with a SIGALRM coming in and doesn't just re-try on EINTR. Can Solaris stdio _really_ be that broken? (Yeah, yeah, it may be "conforming". It's also so incredibly programmer-unfriendly that it's not even funny)

That would be truly insane. Can somebody with Solaris check what the following patch results in...

		Linus

----

And Linus Torvalds writes:
 - 
 - so it really really looks like fgets() would have problems with a SIGALRM 
 - coming in and doesn't just re-try on EINTR. Can Solaris stdio _really_ be 
 - that broken? (Yeah, yeah, it may be "conforming". It's also so incredibly 
 - programmer-unfriendly that it's not even funny)

Yes, it is that broken. I haven't encountered the problem consistently in git myself, so I can't tell you if the patch works. Google finds similar reports and patches for BOINC, ruby, and a few other projects.

Solaris folks will say you should be using sigaction with SA_RESTART. IIRC, SA_RESTART isn't guaranteed to be there or work, but all the systems I deal with right now have it. So an alternate patch for this one use is appended... Other uses of signal could be changed to sigaction, too. And progress_update "should" be sig_atomic_t.

Passes the pack-objects tests, but I can't make the problem happen on demand. (I have seen it occur before, but never during make test, and I'd not tracked it down...)

Jason ----

On Sun, 2 Apr 2006, Jason Riedy wrote:

I think we might as well do that _too_.

However, once you use "sigaction()", you don't need to re-arm the signal handler any more, so I'd suggest a simpler patch like this instead..

Junio, I think this confirms/explains the Solaris breakage.

I'll re-send the "anal stdio semantics" version of the patch on top of this in the next email.

			Linus
----
Subject: Fix Solaris stdio signal handling stupidities

This uses sigaction() to install the SIGALRM handler with SA_RESTART, so that Solaris stdio doesn't break completely when a signal interrupts a read.

Thanks to Jason Riedy for confirming the silly Solaris signal behaviour.

Signed-off-by: Linus Torvalds <torvalds@osdl.org>
----

This is the "letter of the law" version of using fgets() properly in the face of incredibly broken stdio implementations. We can work around the Solaris breakage with SA_RESTART, but in case anybody else is ever that stupid, here's the "safe" (read: "insanely anal") way to use fgets.

It probably goes without saying that I'm not terribly impressed by Solaris libc.

Signed-off-by: Linus Torvalds <torvalds@osdl.org>
---

This is the same one that I already sent out, but re-diffed, and with a proper commit message.

Not tested on Solaris.

Junio - I think that I forgot to Cc: you on the 1/2 patch, but you'll see it on the git list.

Linus Torvalds <torvalds@osdl.org> writes:

Thanks.

It's good that I can say "Oh, I think this is the part that is broken, but I am going to bed" to find the problem solved by capable others when I wake up the next day. Global distributed development process at the finest, although I suspect Jason, Linus and myself are all in the same timezone ;-)

Did you mean this as a real change or a demonstration? The sigaction change is a real fix, but somehow I find this one similar to the "(void*) NULL" thing you objected earlier (which was not merged because I agreed with your argument)...

On Sun, 2 Apr 2006, Junio C Hamano wrote:

I don't have any really strong opinions on it. I think that any libc that needs the "ferror()" test + EINTR loopback is totally broken. I would happily say that people should just not use a development platform that is that horrible.

But the fact that Solaris actually had that as a real problem (never mind that we could work around it another way) just makes me go "Hmm..".

So I _think_ we're safe with just the "sigaction()" diff. Neither of the patches _should_ make any difference at all on a sane platform.

		Linus

And Linus Torvalds writes:
 - 
 - I don't have any really strong opinions on it. I think that any libc that 
 - needs the "ferror()" test + EINTR loopback is totally broken. I would 
 - happily say that people should just not use a development platform that is 
 - that horrible.

If you consider stdio to be a low-level wrapper over syscalls that only adds buffering and simple parsing, then passing EINTR back to the application is a sensible choice. I wouldn't be too surprised if L4, VxWorks, etc. do something similar.

 - So I _think_ we're safe with just the "sigaction()" diff.  Neither of the 
 - patches _should_ make any difference at all on a sane platform. 

Anyone with an older HP/UX care to try these patches? HP/UX may not be sane, but I think it may lack SA_RESTART. I don't know if stdio calls need restarted, though.

Jason

On Sun, 2 Apr 2006, Jason Riedy wrote:

No, returning EINTR is insane, because stdio has to do re-starting of system calls by hand _anyway_ for the "short read" case.

EINTR really _is_ 100% equivalent to "short read of zero bytes" (that literally is what it means for a read/write system call - anybody who tells you otherwise is just crazy).

So any library that handles short reads, but doesn't handle EINTR is by definition doing something totally idiotic and broken.

Now, I agree that somebody else might be broken too. I would not agree that it's "acceptable". It's craptastically bad library code.

I'd assume that older HPUX is so BSD-based that all signals end up restarting. That was the BSD signal() semantics, after all.

			Linus

Might as well ape the sigaction change in read-tree.c to avoid the same potential problems. The fprintf status output will be overwritten in a second, so don't bother guarding it. Do move the fputc after disabling SIGALRM to ensure we go to the next line, though.

Also add a NO_SA_RESTART option in the Makefile in case someone doesn't have SA_RESTART but does restart (maybe older HP/UX?). We want the builder to chose this specifically in case the system both lacks SA_RESTART and does not restart stdio calls; a compat #define in git-compat-utils.h would silently allow broken systems.

Signed-off-by: Jason Riedy <ejr@cs.berkeley.edu>

---

 Makefile    |    7 +++++++
 read-tree.c |   28 +++++++++++++++++++---------
 2 files changed, 26 insertions(+), 9 deletions(-)

521dc260ea90a23d58a4e920203af5035ca1a673

On Sun, 2 Apr 2006, Jason Riedy wrote:

Looks good. I didn't realize we had the exact same code duplicated. I guess it's small enough that there isn't a huge win in moving it to some common file..

Does somebody have access to Solaris to verify that this all actually does fix it? I obviously believe it will, since this just explains the symptoms to a tee, but it would still be good to have an actual confirmation by somebody who has access to a Solaris environment.

I do believe that we already require POSIX.2 functionality (regex, fnmatch, C90 compiler), which implies that git probably wouldn't compile anyway on things that are _really_ ancient.

I think SA_RESTART was part of the original POSIX.1 specs, so anybody that doesn't have it is likely to not have a lot of other things we rely on too. There are other SA_* flags that aren't as standard, but I'd expect SA_RESTART to be everywhere (or it likely doesn't have sigaction() at all..).

But hey, I certainly don't have really old HP-UX to test either.

		Linus

Jason Riedy <ejr@EECS.Berkeley.EDU> writes:

What am I missing...?

On Sun, 2 Apr 2006, Junio C Hamano wrote:

I don't think this part is worth it at least for now.

If there really are systems without SA_RESTART, they probably don't have sigaction() either, so #defining SA_RESTART to zero likely won't help.

But somebody can prove me wrong..

		Linus

On Sun, 2 Apr 2006, Linus Torvalds wrote:

While it shouldn't actually matter, I just realized that this incredibly anal sequence isn't actually quite anal enough, sinceit really should have a "clearerr(stdin)" for the continue case when we ignore the EINTR thing.

Otherwise, some stdio implementation might just decide to continue to return NULL from fgets(), since the error indicator is technically sticky. I don't know if they do so, but it's entirely possible.

So I'd almost suggest something like

	char *safe_fgets(char *s, int size, FILE *stream)
	{
		for (;;) {
			if (fgets(s, size, stream))
				return s;
			if (feof(stream))
				return NULL;
			if (!ferror(stream))
				die("fgets returned NULL, not EOF, not error!");
			if (errno != EINTR)
				die("fgets: %s", strerror(errno));
			clearerr(stream);
		}
	}

which sbould then hopefully work as a sane fgets()-replacement for broken environments.

(And then we can just do

	while (safe_fgets(..))

like normal people again, and not be afraid of strange stdio implementations).

		Linus

Linus Torvalds wrote:

There seems to be a whole bunch of places where we use naked write()s where xwrite or fwrite would be a lot more appropriate. The ssh-* files seem to be particularly offensive in that way.

There are also a number of places which call xwrite with the apparent belief that returning short is an error (e.g. blame.c). This as far as I know the more common definition of xwrite(), but is *not* the one used in git -- the one in git only guarantees that at least one character is written.

	-hpa

The sigaction() patch cooked by Linus and Jason are in "next", to fix pack-objects breakage started between 1.2.2 and 1.2.3.

Once this is confirmed to fix the problem on Solaris boxes, I'd like to include and do an 1.2.5, just in case OpenSolaris folks would want to play with it, and also put them in the "master" branch.

I have an access to a not-so-well-maintained Solaris box at work, and built the relevant parts with somewhat stripped down configuration to run the test. The "master" version without the sigaction() patches exhibits the symptom Oejet and I observed the other night, and "next" seems to fix it. I am reasonably happy.

And Junio C Hamano writes:
 - Once this is confirmed to fix the problem on Solaris boxes, I'd
 - like to include and do an 1.2.5, just in case OpenSolaris folks
 - would want to play with it, and also put them in the "master"
 - branch.

I haven't run into the problem since, but I almost never saw it in the first place. Have you been able to trigger it reliably? Maybe I'm "lucky" that my Sun's so slow...

Jason