Terminate immediately when allocation fails #9856
Conversation
|
|
@sipa grr, works for me locally. I'll try adding the include, but I'm guessing that libstdc++ didn't support it for 4.8 :( |
theuni
force-pushed
the
bad_alloc_terminate2
branch
2 times, most recently
from
f4facff to
faf7cb5
Compare
|
Why not make prevector throw std::bad_alloc or std::length_error? That way it's at least recoverable in future code, and makes it a bit more "API Consistent" with std::vector... |
theuni
force-pushed
the
bad_alloc_terminate2
branch
from
faf7cb5 to
420f170
Compare
|
@JeremyRubin The issue in #9854 is that exactly that bad_alloc is thrown but not handled correctly. We don't actually attempt to catch bad_alloc specifically anywhere. And logically it doesn't make sense to... if we've got an allocation problem, we need to shut down. |
|
The default action of exceptions is to abort the program, no? So we must be catching it at least indirectly... Why not just add an explicit catch for bad_alloc there? (or ideally, be specific about what exceptions we want to catch..) |
|
ACK on the assert() in prevector. I really don't like setting this global handler that crashes the program. I hope this is a temporary measure for 0.14? |
|
Thinking of it, no, using assert for error handling is a bad idea. We should support compiling without assertions at some point. |
|
@laanwj I hope we do keep this longer-term, actually...std::bad_alloc, in the design of our entire codebase, seems massively dangerous to me (and I've wished we could do something like this for a while - just didn't know it existed). I'm happy if we remove it later, but the amount of auditing we'd have to do to check everywhere we call new, even through stl, seems like a rather significant effort.
…On February 25, 2017 2:36:44 AM EST, "Wladimir J. van der Laan" ***@***.***> wrote:
ACK on the assert() in prevector.
I really don't like setting this global handler that crashes the
program. I hope this is a temporary measure for 0.14?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
#9856 (comment)
|
|
@TheBlueMatt okay, yes, after discussion IRC I agree seems the least bad solution for now :/ Edit: so to be clear, my concern with a jackhammer solution like this is that not all allocations are created equal. An allocation of a 4 GB buffer somewhere in the program may fail, but that may be perfectly fine if it can continue with a smaller buffer. It doesn't mean the rest of the application, which will never allocate a big buffer like that, is in a bad state. Specificallly we care deeply about bad_allocs thrown by consensus and block/utxo handling code. Error handling in Bitcoin Core has always been a bit flakey, and in a way his feels a bit like throwing in the towel on reasoned error handling completely.So that's why my angry reply. Sorry for that. But we need this now to avoid corruption problems. I think we should continue with this. |
|
Ah, I see. I was going to suggest get_new_handler, then I actually read the above discussion ;) I'm mostly just uncomfortable with using assert for error handling. Can we not implement something similar to get_new_handler ourselves for unsupported platforms? |
|
@laanwj yea, I totally agree there are places we don't want this (mostly deserialization comes to mind), but I believe we mostly fixed that by now?
…On February 25, 2017 4:10:18 AM EST, Jeremy Rubin ***@***.***> wrote:
Ah, I see. I was going to suggest get_new_handler, then I actually read
the above discussion ;)
I'm mostly just uncomfortable with using assert for error handling.
Can we not implement something similar to get_new_handler ourselves for
unsupported platforms?
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#9856 (comment)
|
src/init.cpp
Outdated
There was a problem hiding this comment.
Before this attempt, you could do fputs(stderr, "Error: Out of memory. Terminating.\n"); which, I believe, does not call malloc. At least it didn't when I wrote my own malloc a few years ago, but it may be different per platform. A nice to have for when a node goes down with no log entries about why.
There was a problem hiding this comment.
I don't think this is threadsafe (ie, a concurrent OOM could cause it to terminate in the middle of a LogPrintf or print two LogPrintfs). I would make the handler have some switch(atomic counter++). Then it is both re-entrant and threadsafe. (note that set_new_handler is threadsafe itself, just is threadsafe in this usage)
I think this sort of implementation should be ok:
static std::thread::id first_entrant{std::this_thread::get_id()};
static bool first_pass {true};
if (first_entrant == std::this_thread::get_id()) {
if (first_pass) {
first_pass = false;
// Single faulted
std::terminate();
} else {
// Double faulted
}
} else {
sleep();
}Either of the above should be safe for concurrent OOM as well as double OOM faults.
There was a problem hiding this comment.
Those are both reasonable suggestions, but imo they're overthinking this a bit. If we OOM, the important thing is to shutdown, logging should be best-effort. I'd rather not complicate this too much.
|
nit: Perhaps we should add a comment to the |
theuni
force-pushed
the
bad_alloc_terminate2
branch
from
420f170 to
d4ee7ba
Compare
|
utACK |
d4ee7ba prevector: assert successful allocation (Cory Fields) c5f008a don't throw std::bad_alloc when out of memory. Instead, terminate immediately (Cory Fields) Tree-SHA512: 699ce8df5b1775a99c71d3cfc952b45da1c0091e1a4b6adfac52d5be6144c3d98f88ac3af90e5c73fff2f74666a499feb4a34434683ce5979814e869c0aeddc3
d4ee7ba prevector: assert successful allocation (Cory Fields) c5f008a don't throw std::bad_alloc when out of memory. Instead, terminate immediately (Cory Fields) Tree-SHA512: 699ce8df5b1775a99c71d3cfc952b45da1c0091e1a4b6adfac52d5be6144c3d98f88ac3af90e5c73fff2f74666a499feb4a34434683ce5979814e869c0aeddc3
Terminate immediately when allocation fails Backport of bitcoin/bitcoin#9856 Closes #1498.
Terminate immediately when allocation fails bitcoin#9856
d4ee7ba prevector: assert successful allocation (Cory Fields) c5f008a don't throw std::bad_alloc when out of memory. Instead, terminate immediately (Cory Fields) Tree-SHA512: 699ce8df5b1775a99c71d3cfc952b45da1c0091e1a4b6adfac52d5be6144c3d98f88ac3af90e5c73fff2f74666a499feb4a34434683ce5979814e869c0aeddc3
d4ee7ba prevector: assert successful allocation (Cory Fields) c5f008a don't throw std::bad_alloc when out of memory. Instead, terminate immediately (Cory Fields) Tree-SHA512: 699ce8df5b1775a99c71d3cfc952b45da1c0091e1a4b6adfac52d5be6144c3d98f88ac3af90e5c73fff2f74666a499feb4a34434683ce5979814e869c0aeddc3
a8b965e Explicitly initialize prevector _union (random-zebra) bdd98e8 [Trivial] Add a comment on the use of prevector in script. (random-zebra) bbaa6e3 Clarify prevector::erase and avoid swap-to-clear (random-zebra) 28a8435 prevector: assert successful allocation (random-zebra) de41cb5 Only call clear on prevector if it isn't trivially destructible (random-zebra) 8784df3 Make CScript (and prevector) c++11 movable. (random-zebra) 83f9ac6 serialize: Deprecate `begin_ptr` / `end_ptr` (random-zebra) c3ecc12 prevector: add C++11-like data() method (random-zebra) 5490aa0 test prevector::swap (random-zebra) 035760e prevector::swap: fix (unreached) data corruption (random-zebra) 0e71400 prevector: destroy elements only via erase() (random-zebra) 9811a68 [Core] Prevector type (random-zebra) Pull request description: Based on: - [x] #1554 This introduces `prevector<N, T>`, a new basic data type which is a fully API compatible drop-in replacement for `std::vector<T>` and uses it for the script, to reduce the considerable memory overhead of vectors in cases where they normally contain a small number of small elements. Original tests in Bitcoin showed a reduction in dbcache memory usage by **23%**, and made an initial sync **13%** faster. Backported from the following upstream's PRs, with only additional edits in the 2nd layer scripts (`masternode-payments`, `masternode-budget`) and in the unit tests (to address the differences in `insecure_rand`): - bitcoin#6914 - bitcoin#7888 - bitcoin#8850 - bitcoin#9349 - bitcoin#9505 - bitcoin#9856 - bitcoin#10534 - bitcoin#11011 - bitcoin#14028 ~~NOTE: Updates to `memusage.h` needed as well, when #1531 is merged.~~ ACKs for top commit: furszy: Tested ACK a8b965e . Fuzzbawls: ACK a8b965e Tree-SHA512: 203b660dd8d9f98780be660dae0ac951766ba438836bd6f0ec921e7749c1a84143f3b920fe49f2cc399f4aa7e763098f78746d3b6ff845bcf913bb13de984bc1
10 participants
Addresses #9854.
This installs a handler for failed allocations. Usually, a std::bad_alloc would be thrown, but we can instead opt to terminate immediately. See http://en.cppreference.com/w/cpp/memory/new/set_new_handler .
Our only manual malloc/free comes from prevector. Ideally we'd just use new[]/delete[] there, but the benchmark shows a noticeable difference, and I'm very hesitant to make that kind of change post-rc2.
As an alternative, this could even just be an assert() for 0.14.
For reference, here's the alternative using new/delete: theuni@0c1b410
Edit: g++/libstdc++ didn't support get_new_handler() yet, so assert it is.