Presumably there's a way to trampoline out with bindings which do not have a C layer? I'm thinking that lg2s will want to convert this into an exception.

What does lg2s need in order to set up a panic handler that can raise exceptions?

/cc @nulltoken

We currently rely on the returned error codes to detect which Exception to throw.

This SO answer and this post seems to say that interop with variadic functions are possible by only declaring overloads.

Let's try this, then!

Ok, I've started digging deeper into this and with the panic callback, we can remove literally thousands of error handling paths that the library simply never visits. It's gonna take some effort (couple days) and I can do it, but first I want to make sure that everybody is OK with panic'ing on OOM.

cc @arrbee, @jspahrsummers, @paulcbetts, @ethomson

From the LibGit2Sharp standpoint, the plan would be call Environment.FailFast from the managed callback.

@nulltoken: Awesome. Just checking that the VS people are ok with that, cc @ethomson

In rugged, I think we can use rb_fatal.

I think my gut reaction would have been to take the opposite approach -- to run tests with some sort of a chaos monkey that inserts OOMs at random points so that we can see what breaks and try to exercise the OOM error propagation codepaths. By saying we are going to take this approach, we are punting on being able to live in certain hosting environments which might be able to react in a more positive way to OOM situation (kick off a garbage collection, somehow come up with more memory and try again, etc.).

Are these highly plausible scenarios? Probably not. For the vast majority of hosts, saying that we support a fail-fast policy only is a reasonable thing to say. What do other high-quality open source libraries do to handle this? We should look to the rest of the community for guidance here.

Interesting @phkelley , I think that you and I have the same concerns but came to a different opinion of the merit of this PR in achieving those goals.

Frankly, I don't think this is particularly different than our current situation which is to return an error code indicating OOM. Now we can hook our custom oom handler programmatically and do a retry.

(Regarding the bindings, though, I think we should be able to configure this there, too - I'm not sure that Environment.FailFast will be appropriate in Visual Studio, for instance ;)

/cc @spraints @psantosl @KindDragon @jbialobr @dahlbyk

BTW, unless I'm wrong there's no "retry" mechanism in giterr_panic() handling. This is rather a "I'm letting know that I just died." fatal notification.

Set the panic handler. This callback is issued by libgit2
during a critical (non-recoverable) error.
This basically means Out-of-memory conditions, which shouldn't
really happen on modern OSes because of overcommitment.
The callback is not supposed to return: ensure to either crash
the running process with the appropriate error message, or
trampoline out of the callstack.

However, that's right, there's the option of throwing a less fatal OutOfMemoryException. There are chances that the handles we're holding to may be in an unstable/partially updated state. So, if the host catch the exception and triggers another operation... well...

"There's something very important I forgot to tell you! Don't cross the streams…
It would be bad… Try to imagine all life as you know it stopping instantaneously
and every molecule in your body exploding at the speed of light."

     —Egon Spengler on crossing proton streams

Throwing instead of Environment.FailFast seems a better option to me.

As you can see from the docs:

the running process with the appropriate error message, or trampoline out of the callstack

You're definitely not required to crash the host, you can raise an exception or simply trampoline and try again.

As for @arthurschreiber's question: what are other libraries doing? Well, from my experience the thing the do (and IMO the best thing you can do) is allowing the user to customize the memory allocator used by the library, so you can e.g. pass the Ruby, Python or .NET VM allocator that supposedly already fails "properly" when OOM.

This is definitely an option for libgit2: instead of panicing, simply let the user set his memory allocator and let him choose how to handle these situations. For both Ruby and Python bindings this would be very nice -- the question is, do you guys have a PTR to the .NET CLR malloc thingie?

You're definitely not required to crash the host, you can raise an exception or simply trampoline and try again.

One have to keep in mind that the previous function call (the one that led to an OOM error) may have exited halfway through the function, without guarantying a clean slate. So, there are chances the "retry" call may work against corrupted data.

I'm 👍 on this, at least for the non-managed code we write on OS X and iOS. On OOM, malloc() will almost always return a "valid" pointer that causes a segmentation fault if dereferenced, so we wouldn't hit the error handling code paths anyways.

A segfault!??! That's some brain damage of the highest order.

@ethomson Unless there's an actual limit applied to the process, OOM is basically impossible on OS X, because swap will just expand as necessary. On iOS, the app gets memory warnings of increasing severity before being jettisoned by the system.

@jspahrsummers I'm okay with it overcommitting, but a segfault seems particularly cruel. It seems like one would be spending a lot of time debugging core files that were really just OOM situations and not attempts to read/write invalid memory, which is what I think of as a classical segmentation violation.

So after thinking about this more, I'm increasingly on the fence about this. Especially since when I've found it unwise to argue with @phkelley because he's usually correct. So I will once again ask a question that myself and others have asked before and that is: why don't we try to clean up and return an error code?

(I trust that this will also solve the problems with static code analysis warnings that this PR is trying to solve in the first place.)

I realize that nobody's particularly excited about this, but let's have a thought exercise if you don't mind.

Let's say I want to malloc a gigabyte of memory. Three things can happen:

1. The system can hand me over a pointer that I can then fill up with 1024^3 bytes as I see fit. This is the happy path.
2. The system can hand me over a pointer because it has some optimistic overcommit assumptions that I'll never actually use this memory and it will kill me later. (Apparently Mac OS actually segfaults, which is seems impossibly confusing to me.)
3. The system can return NULL from *alloc. I think that this is much more likely to happen in Windows than Linux and Mac OS which tend to be generous about overcommitment. But this is a very unscientific study I've performed.

Obviously option 1 is what we want and option 2 is impossible to control for so we're really only talking about option 3. I don't think it's a foregone conclusion that malloc failing is impossible to recover from. See for example #2004 where we tried to malloc insane amounts of memory and failed. Here freeing everything and returning would have been very successful.

You could argue that this was simply a bug in the library (and that's true) but I can imagine that some operations - diff, blame and merge all come to mind - legitimately require a decent amount of RAM to get the job done and them running out of memory is not an indication that future mallocs would fail.

Let me create a complete hypothetical that somebody pushes up a whole bunch of hundred megabyte files and then tries to annotate them. Let's assume that this will run out of memory in a way that malloc fails halfway through the annotation such that some huge amount of RAM is currently sitting allocated when it fails. I don't think this is a particularly wildly impossible scenario.

I would argue that the current philosophy of "if malloc fails then we're totally fucked and never going to try to recover" is very much lacking for somebody who wants to host libgit2 in a long running process. I usually try to talk about our various products in the abstract, but more concretely: I think you guys spin up short-lived processes on your backend, while Team Foundation Server is long running and basically inbuilt to IIS. For us, this solution is really bad because we've got all this memory now mallocd that we can't free.

To get around this we built some crazy machinations to make our managed code talk to libgit2 over a process shim that's equal parts clever and disappointing and I would have rather used pinvoke.

So to sum up my feelings about this after more reflection: this PR is fine, but it doesn't really solve any of my problems. It makes my dealing with OOM in native code a little bit more awkward (but nothing to cry about, I don't need to try to longjmp back to anything.) I feel like we're making these changes just to make a static code analyzer happy and the net benefit to the users of the library is unchanged or slightly negative.

I actually read through all of that! Also I partly agree.

From my point of view: the current situation with OOM is bad. Overcommitment issues aside: assuming that NULL pointers will never be returned, and in the off case that they do get returned, return quickly without bothering to clean up anything, that leaves us with the both of both worlds.

1. In the case of an actual OOM error, we return an error code, but we haven't really cleaned up any memory, so further retries are kind of pointless.
2. By default, we simply have a lot of extra checks and error paths that we could skip altogether because they are basically never walked.

As @ethomson very well explains on his comment, there are two ways out of this:

• Proper error handling. This means adding a couple hundred new error paths to make sure that we do clean up as much memory as we can on OOM. This is a huge undertaking that will add a lot of complexity to the library (complexity we've gotten rid of in the past). It has the benefit of being more "conceptually elegant" and clean, despite the fact that it clutters the codebase.
• Fuckit.jpg and panic out. This is what this PR does, going in the polar opposite direction. Its main benefit is that it removes hundreds of code paths (or will remove -- I still need to finish it if you think this is going in the right direction) that currently add a lot of clutter to the library. There are a lot of internal APIs (pools, vectors, strings and a large etc) that return int error codes solely on OOM, and they could very well be void calls that never fail like they are in git.git. This makes the codebase substantially cleaner and makes it much more straightforward to port stuff from git.git since the code there also cannot fail on OOM.

From my (GitHub's) point of view, the panic way is a much better way to go about this, mainly because our long-running code runs on Linux and overcommits like fuck, so the library is essentially full of hundreds of code paths that we never take and make expanding it harder. FWIW we don't spawn processes per request, all of our daemons are long running and monitored, and they very rarely* get OOM-killed by our monitoring tool, despite having pretty tight artificial OOM limits set.

On another note, solving the Coverity warnings would take a line (simply add a panic pragma-comment on the OOM error setter), so this PR is most definitely not related to that. The only goal here is removing error code paths that don't get used and clutter the library so we have a cleaner understandings of the real error paths that do happen often and require proper cleanup.

*there is a set of processes that gets killed very often, but this is because of issues when we bring blobs into Rubyland and the Ruby GC. I don't recall ever seeing a process die because of libgit2 itself.

For server-side, restarting the process isn't too much of an issue, but I wonder about client/desktop apps. If a user tells it to open a huge blob which doesn't fit in memory, this would kill the app (or somehow make the app recognise this in the callback). This would not be a case of oom, but asking for too much data. As we don't currently have a way to stream a large object out of the odb, there isn't really a way to work around this (other than maybe refusing to load large blobs...)

On another note, solving the Coverity warnings would take a line (simply add a panic pragma-comment on the OOM error setter), so this PR is most definitely not related to that.

Thank you for this clarification. I obviously made some assumptions here (given the coincident timing with the coverity push) and I'm glad to realize that I was mistaken.

@carlosmn 's point addresses my concerns nicely. If I run out of memory on my server, I definitely want that process to die a horrible death and if I'm given enough notice to print a nice little error to the user telling them why, all the better. But when Visual Studio just goes poof, I get very angry emails. And if it goes poof because somebody did something that they probably didn't even realize was silly, like putting a DVD image in their git repo, that's when I get really nasty bug reports.

(We call these "DTS"es which stands for "days to solution", which is indeed a measurement of how long we have to fix them.)

I have mixed feelings about this.

I like the idea that the compiler would be able to generate much cleaner code for the 99.999% case where these allocations don't fail. I also think it would be sad to have to add hundreds, if not thousands, of lines of extra code to "handle" these types of errors that almost never occur when for most allocations a failure will not be recoverable (i.e. if you fail trying to allocate a 16 byte object, you are probably well and truly fucked).

On the other hand, I see that a) for large allocations on platforms that don't deal well with fragmentation and memory overcommitting, it is entirely possible for an allocation to fail and yet the app could continue, and b) for many desktop apps, even with a failure, it will decrease the appeal of libgit2 if the library can exit underneath you.

I'm trying to think of alternatives. I can think of a few paths forward:

1. Go with this PR and remove the checking, but extend with custom allocator so that an app can attempt to GC or what have you.
2. Decide to fully handle allocation errors with full cleanup, detecting and actually freeing allocations that have occurred. There are a few places in the library that already do this and I've found that (since free(NULL); is a no-op), this is not necessarily so bad, although it adds a bunch of conditionals into code that will never be run.
3. Stick with the "no cleanup" policy we have now, but still detect errors. We could potentially amend this policy in a minor way, such as "cleanup allocations larger than 128 bytes" or something like that. This may encourage us to either coalesce allocations and reorder and/or defer them until other errors are checked.
4. Try a variation of this approach with more explicit support for using setjmp and longjmp to resume operations, adding some callbacks that let you set jump back / resume points inside the library. I'm not sure if this is actually worthwhile to pursue, but it's just a thought I had.
5. As a variation on the "cleanup over size X", we could go with this PR and use a "check allocations over size X" where we eliminate checks on smaller fixed length allocations (gaining some of the code simplification for cases where a failure is most likely unrecoverable) and handle the larger cases (big blobs, big trees, big diffs) explicitly and really make the effort to clean up those cases thoroughly.

I guess the "do something different for large vs. small" is somewhat orthogonal to the "check and cleanup vs abort and (optionally) jump" decision, except that, to me, the abort/jump is more acceptable if I felt like a large blob wouldn't kill a running process.

I hate to introduce rules into libgit2 that require us to code in different ways for minorly different circumstances, but in this case, I think it could gain us a large chunk of the simplification that comes from this PR while preserving a large part of the robustness of checking allocation failures on large awkward objects.

I must say, with your arguments exposed that option #5 is very appealing to me. If we end up in a situation where a 16byte allocation fails, they well, we're fucked, but I can definitely see how we could fail to load a 4gb blob (particularly on a 32-bit system).

I think there are paths that need to be checked for errors much more aggressively (such as blob loading), while others need no checking. Does this kind of middle-ground make somebody uneasy?

One downside that I can think of w.r.t. a mixed policy is that it becomes unclear what to do with things like git_buf, or rather git_buf probably has to stick with the more conservative check-all-allocations policy since it it possible that the buffer will be loaded with data from a blob (or at least with a file on disk). Of course, git_buf does very little inside - it is more the user of git_buf APIs that has to decide what to do.

I'm not sure what the policy should be about things like the array of pointers in a git_vector or the arrays inside a hashtable. A vector can grow pretty long, so probably we have to check it, but if a vector extension fails, the chances of it being recoverable are low.

Maybe a good way to think about this is that buffers, vectors, hash tables, and pools are all really just custom allocators internal to the library and they should probably do checks, but the callers of those custom allocators are free to PANIC if it seems appropriate for the situation.

Hopefully, these things would be uncommon corner cases and most allocations would fall more clearly into the Panic vs. Check categories. Related: I would propose that file names and paths go into the Panic category (even though they are technically of unknown length). I'm somewhat torn on things like commit messages, tree entries, etc., but I'm sure we can make spot judgements and still get a lot of cleanup in the library.

So I understand the appeal (indeed, I don't really care whether we try to free an OID before we fail). But it's sad that would need two completely different mechanisms for reporting OOM and have to deal with them separately.

If we're going to make a distinction between big and small OOM failures (discounting for a moment that I'm not sure what that line is), I'd really rather that we return OOM for both.

If we're going to make a distinction between big and small OOM failures (discounting for a moment that I'm not sure what that line is), I'd really rather that we return OOM for both.

I think this is kind of the option 3 that I listed - institute new cleanup policies that make sure we don't leave large objects behind, but preserve the OOM return that we have currently. Is that right? Just making sure I understand what you'd prefer.

@arrbee that's correct. Sorry, I wanted to be explicit since option 3 lists two parts.

I'm really in favor of option 2, even though it's a pain in the ass. (At least a bit of a pain in the ass, it's not like we don't already have a bunch of failure cases that do frees that are hard to test and / or will never get hit!)

But if we're going to do some hybrid approach then at least don't make me deal with two totally different code paths for what I perceive as a user to be the same thing.

I just wanted to chip in and say that I am also in favor of option 2 even though it is a pain in the ass. On Windows, malloc always returns 0 when there is no more memory. None of this overcommit nonsense :)

Normally I'm 👍 on an idea like @vmg is proposing, since there's often nothing Generally Useful you can do in OOM other than die, at least in user mode.

However, in this case, libgit2 itself could be the one who causes the OOM because they try to open a gigantic repo, and it's feasible that, if libgit2 canceled the operation and freed all the memory associated with it, the application could continue running in a sane state.

I'm now contemplating whether we can do something tricksy like defining a git_oom_t return type and some clever macros for checking that would allow a compile time switch between checking and panicking. Code that wants compact flow and can deal with panic would use one option and we could implement full checks and cleanups for apps that need it. Just a thought. More tomorrow once I've considered the error of my ways...

Okay, so I tried out implementing something like:

#ifdef DIE_ON_OOM

typedef void git_oom_t;
#define GITERR_CHECK_ALLOC(ptr) (void)(ptr)
#define GITERR_OOM_RETURN(val) return

#else

typedef int git_oom_t;
#define GITERR_CHECK_ALLOC(ptr) if (ptr == NULL) { return -1; }
#define GITERR_OOM_RETURN(val) return val

#endif

and then started to update the buffer code to try it out and it did not work out. Having functions sometimes have a checkable return value and sometimes not just turns out to require too much macro bullshit (IMO) to be worth it. It just clutters the codebase. I think it could be done, but at too high a cost in maintainability.

@arrbee I don't think the issue is the complexity of the compiled code, but rather the source. Writing two code paths and switching between them with macros wouldn't solve that problem anyways.

Definitely agree - I guess I was just seeing if we could do something that would be not significantly different from what we do today (which is not too burdensome) that would let you flip between panic-style and error-check-style. But since the main advantage of panic-style is getting rid of this handling code, it is, of course, just a bad idea.

@phkelley asked what other "high-quality" libraries do in this situation, so I looked up what glib does, on top of which the whole gnome ecosystem (and a lot around it) sits.

It provides two families of functions, g_malloc() and g_try_malloc() (and equivalents for the other ones), the former aborts the program on failure, whereas the latter will return NULL, like malloc() does. This allows the programmer to decide whether they are allocating something small that we simply cannot do without (like a config value, or the name of something), or whether it might be something larger and we might in fact be able to recover from failure (like loading a blob). This is essentially @arrbee 's option 5, but checking according to the importance/types of the allocation rather than the size.

Unfortunately in the library we often use git_buf for both types, which would make something like this more complex, but we should be able to use an aborting allocator in many places and reduce a lot of the complexity which this PR tries to.

Qt, which is the platform for the other major desktop, uses C++ and thus does whatever the new operator does, which is again OS-dependent and may throw std::bad_alloc on systems which do not overcommit.

I would also point out that we're not guaranteed that the 5 byte malloc failure indicates a tragic condition that we're out of system memory. We could have just mallocd a gig successfully before that. Not likely, I admit, but I just don't like using the size of the allocation as the switch for whether we die or return an error code.

So I was poking around at this today. My assumption has been that it shouldn't be too onerous to add cleanups throughout the codebase since a lot of functions had to handle different types of errors and clean up memory for them. Indeed, this is mostly what I'm finding. Certainly there are functions that never had error checking that now need some, but I'm feeling fairly positive that adding cleanup in would be a positive step that wouldn't overwhelm us with additional code.

One thing that might improve this is a signature change on allocators so that they return an error code. At present:

ptr = git__malloc(42);
GITERR_CHECK_ALLOC(ptr);

if ((error = git_some_fn()) < 0)
    goto cleanup;

With a jump into the cleanup, this becomes:

if ((ptr = git__malloc(42)) == NULL) {
    error = -1;
    goto cleanup;
}

if ((error = git_some_fn()) < 0)
    goto cleanup;

With a change of the signatures, we could:

if ((error = git__malloc(&ptr, 42)) < 0 ||
    (error = git_some_fn()) < 0)
    goto cleanup;

cleanup:
    git__free(ptr);
    return error;

I think this will result in fewer LOC and is very straightforward.

How much do y'all hate this idea?

If we were to do that, we would probably want to convert lots of the internal allocation functions to use the new patterns as well (e.g. diff_delta__alloc, diff_list_alloc, git_pool_malloc, etc). That's all fairly systematic, but it will end up being a pretty big project, I suspect.

I do not hate this idea. I think it's a pretty good one.

So this turned out to be much less useful than I had hoped - I did this in #2080 which I think was net neutral at best, but probably actually a negative.

I remain bullish about cleaning up memory on OOM, though. #2080 makes me think that there are only a few places that would get hairy. And if I can be completely honest, if we get 99% of the way there, that would be Good Enough for Me. I would like to have not leaking on the OOM case the goal to strive for, but we should be willing to admit that 99% is probably Good Enough here.

That is to say that I'm sure that we have memory leaks in odd failure cases now that are hard / impossible to reproduce. I don't think we should waste our time trying to get to 100% code coverage in the hopes of stomping all of these out. My position would be accurately boiled down as: treat OOM cases like any other failure in the code.

I believe the current thinking is not to go in the P A N I C direction. Should we close this out?

Yes. Thanks for the discussion, all of you.