In what scenarios can std::set::erase(const key_type& key); crash a process?

[Christopher Pisz]

In what scenarios could std::set::erase( const key_type & key) crash a process?

I received a crash dump pointing to a block of code where erase is called on a set and the process crashed. I and several of my peers cannot find a thing wrong with the code.

I think we are all familiar with the scenario where if you iterate through a container and call erase using the iterator without updating it with the result, that you have invalidated an iterator. However, I am not calling erase with an iterator, but with a key;

I've tested calling erase on the same key multiple times and it seems just fine.

I've also tested Add and a Remove methods which insert and erase from the set, in 3 threads on random intervals with a set of keys, and cannot get anything bad to happen.

I've also checked comparators in the value type used for the set and they are correct, as well as simple. Both operator < and operator == are defined correctly.

My set is accessed in multiple threads, but there is indeed a lock_guard at the top of the Add and at the top of the Remove methods.

The docs state that erase will not throw. Yet, here I am.

Anyone have any ideas on what else one could look for?

I am aware that I am not posting code, but my company would greatly frown on that. I completely unable to reproduce the problem in a minimal compilable example. All I have to go on, is that is where the crash dump is pointing every time, on some 50 machines.

I've also read over:

How can std::set::erase(const key_type& key) cause segfault?

std::set<T>::erase(key). What if key isn't there?

and those are not the problem.

[Ben Bacarisse]

Christopher Pisz <christo...@gmail.com> writes:

> In what scenarios could std::set::erase( const key_type & key) crash a
> process?

The obvious one is where the compare function goes wrong in some
catastrophic way.

But much more common would be a bug somewhere else but the code only
falls over when the erase function gets called.

Can you at least give the error message? It might help to know what
sort of "crash" you mean.

<snip>

> I've also checked comparators in the value type used for the set and
> they are correct, as well as simple. Both operator < and operator ==
> are defined correctly.

OK, but you need more. You need to be sure either that they work even
when the objects being compared are invalid, or you need to be sure that
the objects can never become invalid.

> My set is accessed in multiple threads, but there is indeed a
> lock_guard at the top of the Add and at the top of the Remove methods.
>
> The docs state that erase will not throw. Yet, here I am.

A "crash" (you are not exactly clear what that means to you) is not a
throw. (Technically, the docs say that your form of erase can throw an
exception, but only from the compare object.)

> Anyone have any ideas on what else one could look for?

Can you run the code under valgrind? Can you compile and run with
something like gcc's -fsanitize=undefined? These are the two I used
most but there are other run-time checking systems that might be
available to you.

<snip>
--
Ben.

[Christopher Pisz]

On Tuesday, October 10, 2017 at 7:45:12 PM UTC-5, Ben Bacarisse wrote:

> Christopher Pisz <christo...@somewhere.net> writes:
>
> > In what scenarios could std::set::erase( const key_type & key) crash a
> > process?
>
> The obvious one is where the compare function goes wrong in some
> catastrophic way.
>
> But much more common would be a bug somewhere else but the code only
> falls over when the erase function gets called.
>
> Can you at least give the error message? It might help to know what
> sort of "crash" you mean.
>
> <snip>
> > I've also checked comparators in the value type used for the set and
> > they are correct, as well as simple. Both operator < and operator ==
> > are defined correctly.
>
> OK, but you need more. You need to be sure either that they work even
> when the objects being compared are invalid, or you need to be sure that
> the objects can never become invalid.

Not sure what you mean here. What do you mean by invalid? the value_type is just a struct with 3 std::strings and an int. I don't see how they could become invalid. I am not dealing with pointers of any kind either.

> > My set is accessed in multiple threads, but there is indeed a
> > lock_guard at the top of the Add and at the top of the Remove methods.
> >
> > The docs state that erase will not throw. Yet, here I am.
>
> A "crash" (you are not exactly clear what that means to you) is not a
> throw. (Technically, the docs say that your form of erase can throw an
> exception, but only from the compare object.)

Sure. I suspect it is an access violation. I don't know how to get windows crash dump to tell me. All I can get is where the execution pointer was at the time. It is pointing to std::set::erase. Further up is a couple iterator methods and a deletor of some sort.

> > Anyone have any ideas on what else one could look for?
>
> Can you run the code under valgrind? Can you compile and run with
> something like gcc's -fsanitize=undefined? These are the two I used
> most but there are other run-time checking systems that might be
> available to you.

Windows code and windows machines. Company also doesn't have any profiling tools of course. I can run the code with warnings on and get 1000s of hits, because I just building an interface plugged into the middle of existing crappy code.

If I can indeed see the same call stack in 50 some odd crash dumps from 50 different machines, without it actually being that code, but because of UDB elsewhere, then at least I can stop looking at the code being pointed to.

[Ian Collins]

On 10/11/17 04:38 PM, Christopher Pisz wrote:
> On Tuesday, October 10, 2017 at 7:45:12 PM UTC-5, Ben Bacarisse
> wrote:
>> Christopher Pisz <christo...@somewhere.net> writes:
>>
>>> In what scenarios could std::set::erase( const key_type & key)
>>> crash a process?
>>
>> The obvious one is where the compare function goes wrong in some
>> catastrophic way.
>>
>> But much more common would be a bug somewhere else but the code
>> only falls over when the erase function gets called.
>>
>> Can you at least give the error message? It might help to know
>> what sort of "crash" you mean.
>>
>> <snip>
>>> I've also checked comparators in the value type used for the set
>>> and they are correct, as well as simple. Both operator < and
>>> operator == are defined correctly.
>>
>> OK, but you need more. You need to be sure either that they work
>> even when the objects being compared are invalid, or you need to be
>> sure that the objects can never become invalid.
>
> Not sure what you mean here. What do you mean by invalid? the
> value_type is just a struct with 3 std::strings and an int. I don't
> see how they could become invalid. I am not dealing with pointers of
> any kind either.

Please wrap!

Strings can have invalid pointers in them, unlikely but possible.

>>> My set is accessed in multiple threads, but there is indeed a
>>> lock_guard at the top of the Add and at the top of the Remove
>>> methods.
>>>
>>> The docs state that erase will not throw. Yet, here I am.
>>
>> A "crash" (you are not exactly clear what that means to you) is not
>> a throw. (Technically, the docs say that your form of erase can
>> throw an exception, but only from the compare object.)
>
> Sure. I suspect it is an access violation. I don't know how to get
> windows crash dump to tell me. All I can get is where the execution
> pointer was at the time. It is pointing to std::set::erase. Further
> up is a couple iterator methods and a deletor of some sort.

Ah, so maybe something in the object you are erasing is corrupt or is
being deleted twice?

--
Ian

[Melzzzzz]

Tree is probably corrupt by the time erase is called.
>


--
press any key to continue or any other to quit...

[Juha Nieminen]

Christopher Pisz <christo...@gmail.com> wrote:
> In what scenarios could std::set::erase( const key_type & key) crash a process?

There's a million possible reasons. Many of them not related to that code at all
(for instance, something somewhere else, perhaps completely unrelated to that
code, may be writing something out-of-bounds, and it happened at that instance
to garbage the memory used by the std::set object.)

[asetof...@gmail.com]

I don't know how they have success in that, but in Axiom I did not find a stopping bug (something unresolved) until now even without a debugger and a memory analizator...
Only using output[] one function as printf print variables on the screen.

In C++ to find bugs is funny good and better than programming (imo) because each single bit of function for library was written by me (so I would to know how system has to run); but I have to use the debugger for find them. In C all bugs are all out library functions but i have to use debugger for to find them too.

[asetof...@gmail.com]

Should be one error erase one void set

[Ralf Fassel]

* Juha Nieminen <nos...@thanks.invalid>

Sounds much like memory corruption elsewhere. In that case it might
help to enable (or not disable) the "Microsoft Safe Iterators" or
whatever they are called in recent releases of Visual Studio.

The related Visual Studio compiler switches are
/D_ITERATOR_DEBUG_LEVEL
https://msdn.microsoft.com/en-us/en-en/library/hh697468.aspx

/GS (Buffer Security Check)
https://msdn.microsoft.com/en-us/library/8dbf701c.aspx

HTH
R'

[asetof...@gmail.com]

I wrote

Should be one error erase one void set

------
Cancel the above post, it is wrong

[asetof...@gmail.com]

I wrote:
In C all bugs are all out library functions but i have to use debugger for to find them too.

----
Above is wrong, the problems are seg fault in the library...

Yes would be better seg fault never be in library but
When use memory as addresses can happen a seg fault in all the places

There is a workaround
use in library all heap memory
first of use some range memory
use a function that say that range is ok for use

Or use a pure objects language without use address

[Alf P. Steinbach]

On 10/11/2017 1:25 AM, Christopher Pisz wrote:
> In what scenarios could std::set::erase( const key_type & key) crash a process?
>
> I received a crash dump pointing to a block of code where erase is called on a set and the process crashed. I and several of my peers cannot find a thing wrong with the code.
>
> I think we are all familiar with the scenario where if you iterate through a container and call erase using the iterator without updating it with the result, that you have invalidated an iterator. However, I am not calling erase with an iterator, but with a key;

Reproduce the issue in a minimal example.

There's a very high change that the problem is your
iteration-with-removal. Why don't you /beep/ post that code.


Cheers!,

- Alf

[Ben Bacarisse]

Christopher Pisz <christo...@gmail.com> writes:

> On Tuesday, October 10, 2017 at 7:45:12 PM UTC-5, Ben Bacarisse wrote:
>> Christopher Pisz <christo...@somewhere.net> writes:
>>
>> > In what scenarios could std::set::erase( const key_type & key) crash a
>> > process?
>>
>> The obvious one is where the compare function goes wrong in some
>> catastrophic way.
>>
>> But much more common would be a bug somewhere else but the code only
>> falls over when the erase function gets called.
>>
>> Can you at least give the error message? It might help to know what
>> sort of "crash" you mean.
>>
>> <snip>
>> > I've also checked comparators in the value type used for the set and
>> > they are correct, as well as simple. Both operator < and operator ==
>> > are defined correctly.
>>
>> OK, but you need more. You need to be sure either that they work even
>> when the objects being compared are invalid, or you need to be sure that
>> the objects can never become invalid.
>
> Not sure what you mean here. What do you mean by invalid? the
> value_type is just a struct with 3 std::strings and an int. I don't
> see how they could become invalid. I am not dealing with pointers of
> any kind either.

I think we're on the same page. You are right that a struct like that
won't become invalid (I had to use a vague term) in normal use, but
something somewhere else might be scribbling over memory it should not
touch. That causes an error somewhere else to show up in erase.

<snip>

> Sure. I suspect it is an access violation. I don't know how to get
> windows crash dump to tell me. All I can get is where the execution
> pointer was at the time. It is pointing to std::set::erase. Further up
> is a couple iterator methods and a deletor of some sort.
>
>> > Anyone have any ideas on what else one could look for?
>>
>> Can you run the code under valgrind? Can you compile and run with
>> something like gcc's -fsanitize=undefined? These are the two I used
>> most but there are other run-time checking systems that might be
>> available to you.
>
> Windows code and windows machines.

There must be similar tools for Windows but since I don't use Windows
I'm in the dark.

<snip>

> If I can indeed see the same call stack in 50 some odd crash dumps
> from 50 different machines, without it actually being that code, but
> because of UDB elsewhere, then at least I can stop looking at the code
> being pointed to.

Not sure what you are saying here. At the moment, I think you have cast
the net very wide.

--
Ben.

[Scott Lurndal]

Christopher Pisz <christo...@gmail.com> writes:
>In what scenarios could std::set::erase( const key_type & key) crash a proc=
>ess?
>
>I received a crash dump pointing to a block of code where erase is called o=
>n a set and the process crashed. I and several of my peers cannot find a th=

>ing wrong with the code.
>

>I think we are all familiar with the scenario where if you iterate through =
>a container and call erase using the iterator without updating it with the =
>result, that you have invalidated an iterator. However, I am not calling er=

>ase with an iterator, but with a key;
>

>I've tested calling erase on the same key multiple times and it seems just =
>fine.

Just a guess, but I suspect something _else_ in the program is
corrupting the storage used by the std::set.

[Christopher Pisz]

I have made several attempts at that and no use of that code in any unit test fails. Not even with multiple threads.

[James R. Kuyper]

So don't use a unit test as your starting point for the "minimal
example". Something in some other part of your program is triggering
this problem, and none of your unit tests includes that "something". So
start with your entire program. Here's a process I've used several
times. It's a long, slow process, but it often helps located even weird
problems like this one:

Save a copy of your program as the "known failure". Don't do anything
with the original.

1. Make a copy of your "known failure" code, and choose some chunk of
that program to remove or replace with a simpler version. If necessary,
choose the chunk randomly, but favor large chunks, and parts of the code
that you suspect have nothing to do with the problem. Every part of your
program that should have executed after the point where it failed is a
good candidate for removal. If the symptoms don't occur inside the data
input routines, consider replacing those routines with code that
initializes your data structures directly. If you can't find anything
more to remove, you're done: you have your "minimal example". Hopefully,
it's small enough to post to the newsgroup.
Since you haven't identified the actual problem yet, feel free, if
necessary, to remove parts you do suspect to have something to do with
the problem - you might be wrong about that, and if so, that's useful
information.
Make sure to remove things cleanly. If part A of the code relies upon
part B having been executed, don't remove B until after you're removed
A; don't remove the '{' that starts a block of code, without also
removing the '}' that terminates it; etc..

2. Test to see if you can can still reproduce the symptoms of the
problem you're trying to solve with the simplified version of the code.
It's not uncommon for the symptoms to change while you're following this
process. That's not a problem - as long as the new symptoms are just as
much of a problem as the original symptoms, you can treat this as a
successful test. However, make sure that the new symptoms are not just
the result of what you removed, due to not removing it cleanly.

3. If the modified code still fails, save it as your new "known
failure". If not, choose a different part to remove the next time
around, possibly a subset of the code you removed this time.

4. Go back to step 1.

If you follow this process, you'll often find that the fact removing a
given part of the code does/doesn't remove the symptoms of your problem
will provide a vital clue as to what the cause of your problem is. You
might very well end up not needing to present your "minimal example" to
anyone else to figure out your problem.

[Jorgen Grahn]

Yeah. I bet that if the OP places a (useless) s.find(key) before the
s.erase(key), he'd start getting reports about crashes there instead.

/Jorgen

--
// Jorgen Grahn <grahn@ Oo o. . .
\X/ snipabacken.se> O o .

[Jorgen Grahn]

On Wed, 2017-10-11, James R. Kuyper wrote:
> On 2017-10-11 08:46, Christopher Pisz wrote:
>> On Wednesday, October 11, 2017 at 4:17:16 AM UTC-5, Alf P. Steinbach wrote:
>>> On 10/11/2017 1:25 AM, Christopher Pisz wrote:
>>>> In what scenarios could std::set::erase( const key_type & key)
>>>> crash a process?
>>>>
>>>> I received a crash dump pointing to a block of code where erase
>>>> is called on a set and the process crashed. I and several of my
>>>> peers cannot find a thing wrong with the code.
>>>>
>>>> I think we are all familiar with the scenario where if you
>>>> iterate through a container and call erase using the iterator
>>>> without updating it with the result, that you have invalidated an
>>>> iterator. However, I am not calling erase with an iterator, but
>>>> with a key;
>>>
>>> Reproduce the issue in a minimal example.
>>>
>>> There's a very high change that the problem is your
>>> iteration-with-removal. Why don't you /beep/ post that code.

...

>> I have made several attempts at that and no use of that code in any
>> unit test fails. Not even with multiple threads.
>
> So don't use a unit test as your starting point for the "minimal
> example". Something in some other part of your program is triggering
> this problem, and none of your unit tests includes that "something". So
> start with your entire program. Here's a process I've used several
> times. It's a long, slow process, but it often helps located even weird
> problems like this one:
>
> Save a copy of your program as the "known failure". Don't do anything
> with the original.
>
> 1. Make a copy of your "known failure" code, and choose some chunk of
> that program to remove or replace with a simpler version. If necessary,

[snip description]

Nice procedure! Sounds obvious, but probably isn't -- until someone
has described it.

However, it sounds like the OP is in that bad place where he cannot
reproduce the problem by himself at all. Perhaps he cannot run the
program in a realistic scenario.

(Side note/rant: one of the most damaging things you can do to a
software project is to make sure developers cannot run the full
system, and neglect to support debugging in the product.)

[Andreas Dehmel]

On Tue, 10 Oct 2017 16:25:07 -0700 (PDT)
Christopher Pisz <christo...@gmail.com> wrote:

> In what scenarios could std::set::erase( const key_type & key) crash
> a process?

Basically just three:
1) Concurrent access to set or key with at least one writer
2) Dangling objects/pointers (either set or key were destroyed
elsewhere; note this also works transitively via destruction of their
owning object(s))
3) General memory corruption, someone else trashes heap blocks

Regarding 1: you mentioned locking of Add/Remove; if these are the only
functions accessing the set, that's fine, otherwise you have to lock
all other accesses too, including read-only ones.

Regarding 2: destruction is usually visible on _some_ thread's stack
(doesn't have to be the one that crashes, and quite often isn't). You
have to look at the strack traces of all threads to rule this out.
Although unlikely, it might also be key going out of scope for some
reason.

Regarding 3: Given that you can't reproduce the problem with unit
tests (and assuming those unit tests also use multiple threads and
similar access patterns), this seems the most likely scenario. My
condolences. On Windows, there is a free tool called
"ApplicationVerifier" which sometimes can help you find issues like
that, I'd try that first on the full application (not a stripped-down
unit test). It's a bit fiddly to set up and can't hold a candle to
Purify, but unfortunately Purify has been de facto dead for years (and
was quite pricey, too).

Good luck,



Andreas
--
Dr. Andreas Dehmel Ceterum censeo
FLIPME(ed.enilno-t@nouqraz) Microsoft esse delendam
http://www.zarquon.homepage.t-online.de (Cato the Much Younger)

[Öö Tiib]

On Wednesday, 11 October 2017 02:25:16 UTC+3, Christopher Pisz wrote:
> In what scenarios could std::set::erase( const key_type & key) crash
> a process?

In too numerous ways.

> I received a crash dump pointing to a block of code where erase is
> called on a set and the process crashed. I and several of my peers
> cannot find a thing wrong with the code.

That sounds like crash dump of release version of program. You
should try to reproduce the issue.

>
> I think we are all familiar with the scenario where if you iterate
> through a container and call erase using the iterator without
> updating it with the result, that you have invalidated an iterator.
> However, I am not calling erase with an iterator, but with a key;

May be the "this" is already destroyed and left scope and you try
to erase from zombie? May be the object stored in set did somehow
corrupt the integrity of tree node that surrounds it? Too numerous
ways even to list.

>
> I've tested calling erase on the same key multiple times and it seems
> just fine.
>
> I've also tested Add and a Remove methods which insert and erase
> from the set, in 3 threads on random intervals with a set of keys,
> and cannot get anything bad to happen.

The std::set has certainly no "Add" nor "Remove" method. I have no
idea what you tested and how its relevant.

>
> I've also checked comparators in the value type used for the set and
> they are correct, as well as simple. Both operator < and operator ==
> are defined correctly.
>
> My set is accessed in multiple threads, but there is indeed a lock_guard
> at the top of the Add and at the top of the Remove methods.

So you are almost certain that it is not a race condition. That
is some ways how to crash less, but still too lot of ways to list.

Note that the whole design where different threads add and remove
elements of shared between threads std::set (even if synchronized)
may mean bad performance, fragmented memory, plus no scalability.

>
> The docs state that erase will not throw. Yet, here I am.

You did not say that it was unhandled C++ exception thrown from
set::erase. You said it crashed. You did not say what it was.
Yes, Windows calls the crashes also "exceptions" but these are
not C++ exceptions. One typically receives something like
"Exception Code: c0000602" with crash report in Windows.

> Anyone have any ideas on what else one could look for?
>
> I am aware that I am not posting code, but my company would
> greatly frown on that. I completely unable to reproduce the problem
> in a minimal compilable example. All I have to go on, is that is
> where the crash dump is pointing every time, on some 50 machines.

Then reproduce it with full program first.

> I've also read over:
>
> How can std::set::erase(const key_type& key) cause segfault?
>
> std::set<T>::erase(key). What if key isn't there?
>
> and those are not the problem.

Yes. Reproduce the issue, reproduce it with version that is
instrumented for debugging and nail it down. It is actually
simple with C++ to hunt down crashes.

[Christopher Pisz]

On Wednesday, October 11, 2017 at 3:32:14 PM UTC-5, Öö Tiib wrote:
> On Wednesday, 11 October 2017 02:25:16 UTC+3, Christopher Pisz wrote:
> > In what scenarios could std::set::erase( const key_type & key) crash
> > a process?
>
> In too numerous ways.
>
> > I received a crash dump pointing to a block of code where erase is
> > called on a set and the process crashed. I and several of my peers
> > cannot find a thing wrong with the code.
>
> That sounds like crash dump of release version of program.

Yes.

> You should try to reproduce the issue.

I've been trying to and am completely unsuccessful in doing so.
It only crashes in production.

> >
> > I think we are all familiar with the scenario where if you iterate
> > through a container and call erase using the iterator without
> > updating it with the result, that you have invalidated an iterator.
> > However, I am not calling erase with an iterator, but with a key;
>
> May be the "this" is already destroyed and left scope and you try
> to erase from zombie? May be the object stored in set did somehow
> corrupt the integrity of tree node that surrounds it? Too numerous
> ways even to list.
>
> >
> > I've tested calling erase on the same key multiple times and it seems
> > just fine.
> >
> > I've also tested Add and a Remove methods which insert and erase
> > from the set, in 3 threads on random intervals with a set of keys,
> > and cannot get anything bad to happen.
>
> The std::set has certainly no "Add" nor "Remove" method. I have no
> idea what you tested and how its relevant.

My Add and my Remove which calls insert under lock and erase(key) under lock.

> >
> > I've also checked comparators in the value type used for the set and
> > they are correct, as well as simple. Both operator < and operator ==
> > are defined correctly.
> >
> > My set is accessed in multiple threads, but there is indeed a lock_guard
> > at the top of the Add and at the top of the Remove methods.
>
> So you are almost certain that it is not a race condition. That
> is some ways how to crash less, but still too lot of ways to list.

There is a lock and the methods that do the insert and erase are only called in two places. I've traced through it many times and don't see anyway it would not be protected by the lock.

> Note that the whole design where different threads add and remove
> elements of shared between threads std::set (even if synchronized)
> may mean bad performance, fragmented memory, plus no scalability.

I'll optimize after it is working.

> >
> > The docs state that erase will not throw. Yet, here I am.
>
> You did not say that it was unhandled C++ exception thrown from
> set::erase. You said it crashed. You did not say what it was.
> Yes, Windows calls the crashes also "exceptions" but these are
> not C++ exceptions. One typically receives something like
> "Exception Code: c0000602" with crash report in Windows.

I don't see any indicator of what it actually is. I suspect an access violation. All I see is the execution pointer when the crash dump was written.

> > Anyone have any ideas on what else one could look for?
> >
> > I am aware that I am not posting code, but my company would
> > greatly frown on that. I completely unable to reproduce the problem
> > in a minimal compilable example. All I have to go on, is that is
> > where the crash dump is pointing every time, on some 50 machines.
>
> Then reproduce it with full program first.

Again, I cannot reproduce it, not even with the full program. However, it happens 100% of the time in production. The only difference really is the amount of load. I cannot simulate that many connections with the hardware given to me.

> > I've also read over:
> >
> > How can std::set::erase(const key_type& key) cause segfault?
> >
> > std::set<T>::erase(key). What if key isn't there?
> >
> > and those are not the problem.
>
> Yes. Reproduce the issue, reproduce it with version that is
> instrumented for debugging and nail it down. It is actually
> simple with C++ to hunt down crashes.

Again, I am unable to reproduce it. If I could reproduce it, I'd probably know what was wrong. I am working on doing that and will continue to, because I don't see how to guarantee it is fixed if we can't make it happen in the first place.

[James R. Kuyper]

On 2017-10-11 17:32, Christopher Pisz wrote:
> On Wednesday, October 11, 2017 at 3:32:14 PM UTC-5, Öö Tiib wrote:

...

>> You should try to reproduce the issue.
>
> I've been trying to and am completely unsuccessful in doing so.
> It only crashes in production.

Then you'll have to debug the problem in the production environment. It
might be difficult to get permission to do so - but if so, ask the
relevant authorities which they'd prefer: letting you debug in the
production environment, or letting the program randomly crash for
unexplained reasons in the production environment. If they prefer the
latter, give them what they want.

How feasible this would be depends precisely upon how your "production
environment" is defined. My production environment doesn't allow me to
send messages to stdout or stderr, but does allow me to send them to a
log file. Debugging printf()s are slower and harder to use than a proper
debugger, but sending their output to a log file should be sufficient to
debug most problems. My production environment does allow definition of
"Archive Sets", and if I use an archive set that's different from the
one used in ordinary production, my input and output files don't
interfere in any way with the regular production system, which uses a
different Archive Set.

[Öö Tiib]

On Thursday, 12 October 2017 00:32:31 UTC+3, Christopher Pisz wrote:
> On Wednesday, October 11, 2017 at 3:32:14 PM UTC-5, Öö Tiib wrote:
> > On Wednesday, 11 October 2017 02:25:16 UTC+3, Christopher Pisz wrote:

... snipping it a bit

> > >
> > > The docs state that erase will not throw. Yet, here I am.
> >
> > You did not say that it was unhandled C++ exception thrown from
> > set::erase. You said it crashed. You did not say what it was.
> > Yes, Windows calls the crashes also "exceptions" but these are
> > not C++ exceptions. One typically receives something like
> > "Exception Code: c0000602" with crash report in Windows.
>
> I don't see any indicator of what it actually is. I suspect an access violation. All I see is the execution pointer when the crash dump was written.

It is odd crash dump that does not state the exception code of crash.
In what form you get it? What reports it?
Access violation exception code I even remember. It is c0000005. That
can also be sometimes represented by decimal forms -1073741819 or
3221225477 by clueless programmers.

... snipping more

> Again, I cannot reproduce it, not even with the full program. However,
> it happens 100% of the time in production. The only difference really
> is the amount of load. I cannot simulate that many connections with
> the hardware given to me.

It must be is worth something to someone that it stopped crashing.
Usually it is worth way more than a rack of windows boxes. If that
is not the case then it is relatively worthless program and there are
really no much point of fixing worthless programs.

[Christopher Pisz]

I am not getting a pop up in visual studio when I have my QA guy slam my box running the process locally. It says, "Visual Studio has hit a breakpoint", however, there is no breakpoint set and exception setting do not say to stop on an exception.

It is pointed to

// TEMPLATE FUNCTION _Debug_lt_pred
template<class _Pr,
class _Ty1,
class _Ty2> inline
constexpr bool _Debug_lt_pred(_Pr&& _Pred,
_Ty1&& _Left, _Ty2&& _Right,
_Dbfile_t _File, _Dbline_t _Line)
_NOEXCEPT_OP(_NOEXCEPT_OP(!_Pred(_Left, _Right))
&& _NOEXCEPT_OP(_Pred(_Right, _Left)))
{ // test if _Pred(_Left, _Right) and _Pred is strict weak ordering
return (!_Pred(_Left, _Right) // !!!!!IT STOPPED HERE!!!!!
? false
: _Pred(_Right, _Left)
? (_DEBUG_ERROR2("invalid comparator", _File, _Line), true)
: true);
}

In xutility.
Does that look like something familiar to anyone, when inside a set?

debug_it_pred<std::less<MyClass> & ptr_64, MyClass & _ptr64, MyClass const &_ptr64(std::less...... is the last thing on the callstack. It goes on for lines and lines.

Sorry for the formatting. All I can use here is Google Groups. I am not allowed to install software, I am not allowed to open most sites, I am not allowed to access production, I am not allowed....you get the point.

[Paavo Helde]

On 12.10.2017 0:32, Christopher Pisz wrote:
> On Wednesday, October 11, 2017 at 3:32:14 PM UTC-5, Öö Tiib wrote:
>> On Wednesday, 11 October 2017 02:25:16 UTC+3, Christopher Pisz wrote:

[...]

> I don't see any indicator of what it actually is. I suspect an access violation. All I see is the execution pointer when the crash dump was written.
>
>
>>> Anyone have any ideas on what else one could look for?
>>>
>>> I am aware that I am not posting code, but my company would
>>> greatly frown on that. I completely unable to reproduce the problem
>>> in a minimal compilable example. All I have to go on, is that is
>>> where the crash dump is pointing every time, on some 50 machines.
>>
>> Then reproduce it with full program first.
>
> Again, I cannot reproduce it, not even with the full program. However, it happens 100% of the time in production. The only difference really is the amount of load. I cannot simulate that many connections with the hardware given to me.

This definitely seems like a multi-threading bug. These are sometimes
notoriously hard to find. Some random thoughts:

The C++ std::set::erase() itself is correct. As you did not find the bug
immediately, then most probably your Remove() is also correct. The bug
is probably elsewhere.

Note that it is not enough if your Add and Remove are protected by a
mutex, all read-only accesses must be protected too. And you cannot
store a set iterator outside of the mutex lock unless you are 100% sure
no other thread will delete the pointed item meanwhile.

Review all your locks. If you are using a RAII mutex lock like
std::lock_guard (like you should), then it is very easy to accidentally
lock it only for 1 line:

std::lock_guard(myMutex);

instead of the correct

std::lock_guard myLock(myMutex);

For reproduction the bug in development:
- find a machine with as many physical cores as you can.
- ensure your program will use as many threads as it can in parallel.
- ensure there is a loop in the main() or similar so it runs indefinitely
- start the program in debugger, stopping on segfaults.
- wait until it crashes, study the call stacks in all threads. You
might need to wait for hours if you are running in Debug or on less
cores than the customer.
- if needed, repeat until the culprit function or thread starts to
pop out statistically in the thread stacks.

HTH
Paavo

[Chris Vine]

On Thu, 12 Oct 2017 10:09:39 +0300
Paavo Helde <myfir...@osa.pri.ee> wrote:
[snip]

> Note that it is not enough if your Add and Remove are protected by a
> mutex, all read-only accesses must be protected too. And you cannot
> store a set iterator outside of the mutex lock unless you are 100%
> sure no other thread will delete the pointed item meanwhile.

A propos of which, my low opinion of online videos as a teaching source
was amplified by a talk I saw Herb Sutter give on C++11 threads a
number of years ago, in which he proclaimed that "const means
thread-safe". In doing so he neglected to mention that the code that
the programmer is examining may hold an object by const reference
and/or only call const methods on it, but in a multi-threaded program
some other thread may hold a mutable reference to it and be concurrently
modifying it. Such modification by another thread virally introduces
thread unsafety unless all accesses having locking provided for them,
including the reads.

He of course new this, but his summarizing of the issue with "const
means thread-safe" for the purposes of his video was highly misleading.

Chris

[Jorgen Grahn]

On Wed, 2017-10-11, James R. Kuyper wrote:

> On 2017-10-11 17:32, Christopher Pisz wrote:
>> On Wednesday, October 11, 2017 at 3:32:14 PM UTC-5, Öö Tiib wrote:
> ...
>>> You should try to reproduce the issue.
>>
>> I've been trying to and am completely unsuccessful in doing so.
>> It only crashes in production.
>
> Then you'll have to debug the problem in the production environment. It
> might be difficult to get permission to do so - but if so, ask the
> relevant authorities which they'd prefer: letting you debug in the
> production environment, or letting the program randomly crash for
> unexplained reasons in the production environment. If they prefer the
> latter, give them what they want.

It's also worth asking the Authorities if you maybe should have
full-scale systems, load generators, and testers and developers using
them.

I spend many years at a place where we built and sold complex
servers. We had all of those aids, and it was money well spent;
we needed them.

> How feasible this would be depends precisely upon how your "production
> environment" is defined. My production environment doesn't allow me to
> send messages to stdout or stderr, but does allow me to send them to a
> log file. Debugging printf()s are slower and harder to use than a proper
> debugger, but sending their output to a log file should be sufficient to
> debug most problems. My production environment does allow definition of
> "Archive Sets", and if I use an archive set that's different from the
> one used in ordinary production, my input and output files don't
> interfere in any way with the regular production system, which uses a
> different Archive Set.

Making systems that can be debugged in production-like setups ... it
pays off to spend some effort on that, too.

[Öö Tiib]

Oh? If something may write into that const then what const it is? It is
not const. The const is promise that nothing writes into it.

When something writes into it then that is entirely different situation
where we need RW locks. Note that RW lock has pile of names:
"readers–writer lock", "shared-exclusive lock", "multiple
readers/single-writer lock", "multi-reader lock" and also "push lock".

In POSIX it is called "pthread_rwlock_t" and in C++ it is called
"std::shared_mutex" since C++17. ;)

You may certainly use it in a way that const-qualified member functions
use lock_shared, try_lock_shared and unlock_shared of std::shared_mutex
and non-const member functions use lock, try_lock and unlock of it.
Then you have both const and thread safety like Herb Sutter said. :D

[Chris Vine]

Your posting is impenetrable. It certainly misses my point.

One thread may reference an object by const reference and another thread
may at the same time reference the same object by non-const reference;
this may happen merely as a consequence of function signatures. The
thread referencing the object by const reference, say as a function
argument, can make no automatic assumptions about whether it can safely
read that object without synchronization.

When Herb said "const means thread-safe" he was not referring to
objects which are _created_ as const objects and so which cannot
lawfully be modified at all, if that was your point. He was
interpreting the provision in the standard which provides that in the
absence of concurrent mutation, a type in the standard library can
safely be read concurrently by two (or more) threads without further
synchronization, and extrapolating from that; and he was referring to
objects which are held by const reference or to which const methods are
applied.

As I said this was a highly misleading summary. Constness is concerned
with something different from thread safety: by itself it guarantees
nothing about thread safety, unless the object concerned happened to be
created as a const object.

Yes, you can lock with read-write locks if the reads significantly
exceed the writes[1]. You can lock with mutexes also. So what? If
you lock correctly, your code is thread safe irrespective of whether
the object concerned is held by any particular thread as const or not.

Chris

[1] However read-write locks perform poorly unless there is writer
priority to prevent writer starvation (which neither POSIX nor C++
guarantees); and their greater complexity compared to mutexes means that
except where reads considerably exceed writes mutexes are usually
better.

[Öö Tiib]

Huh? I mostly wrote about RW locks and those are the tool for
situation where one thread (to what object belongs) may sometimes write
into it while other threads (that have const references to it) often
read it.

> One thread may reference an object by const reference and another thread
> may at the same time reference the same object by non-const reference;
> this may happen merely as a consequence of function signatures. The
> thread referencing the object by const reference, say as a function
> argument, can make no automatic assumptions about whether it can safely
> read that object without synchronization.

I understood that Herb Sutter meant such design style. It can't be that
he literally meant that we get thread safety for free by using const
keyword and synchronization is not needed with such.

For example of design style one can make it so that object does not
return non-const references to its members. It is achievable merely by
not using such function signatures in interface of class. As result
non-const references to its members do not spread all around the board
and so every by-passer can't modify those members.

> When Herb said "const means thread-safe" he was not referring to
> objects which are _created_ as const objects and so which cannot
> lawfully be modified at all, if that was your point.

It wasn't my point. My point was that communication between threads
has to be under control of design like communication between objects
has to be under control of design.

Now lets start from what I think has to be hurtingly obvious. If
something passes const references to some object to other thread
then that sure has to mean promise to not destroy and deallocate
that object when the other thread is processing it. Or how else?

Further it also means promise not to modify that object anymore
or it means promise that the object synchronizes accesses of it
itself (for example by using atomics). That is also obvious?

If passer can't make such promises then it should pass copies of
object (instead of references) to that other thread. Period.
That is how I understood Herb Sutter.

> He was
> interpreting the provision in the standard which provides that in the
> absence of concurrent mutation, a type in the standard library can
> safely be read concurrently by two (or more) threads without further
> synchronization, and extrapolating from that; and he was referring to
> objects which are held by const reference or to which const methods are
> applied.
>
> As I said this was a highly misleading summary. Constness is concerned
> with something different from thread safety: by itself it guarantees
> nothing about thread safety, unless the object concerned happened to be
> created as a const object.

It must be was put up in unfortunate and misleading manner because
as I understand it ... you were mislead.

In real software majority of objects are actually immutable (may have
short initialization immediately after their creation). Passing not
synchronized const references to such objects between threads is safe
and easy and works.

However here is also a trap. Software will evolve and the immutable
things become "mostly immutable but sometimes configurable". Result
is potential race condition during that "configuration".

> Yes, you can lock with read-write locks if the reads significantly
> exceed the writes[1]. You can lock with mutexes also. So what? If
> you lock correctly, your code is thread safe irrespective of whether
> the object concerned is held by any particular thread as const or not.
>
> Chris
>
> [1] However read-write locks perform poorly unless there is writer
> priority to prevent writer starvation (which neither POSIX nor C++
> guarantees); and their greater complexity compared to mutexes means that
> except where reads considerably exceed writes mutexes are usually
> better.

Yes, but that all is about performance of primitives and possible
micro-optimizations by exchanging those. It is more interesting how
your confusion about that big picture did arise. The "const" keyword
does mean nothing like "private" only some compile-time checks. So
how you assumed it carries thread safety unless programmer puts it
to carry that?

[Chris Vine]

On Thu, 12 Oct 2017 22:40:02 -0700 (PDT)
Öö Tiib <oot...@hot.ee> wrote:
> On Friday, 13 October 2017 01:31:35 UTC+3, Chris Vine wrote:

[run]

> > Your posting is impenetrable. It certainly misses my point.
>
> Huh? I mostly wrote about RW locks and those are the tool for
> situation where one thread (to what object belongs) may sometimes
> write into it while other threads (that have const references to it)
> often read it.

The claim was not that "const means thread-safe if you use RW locks or
mutexes". The claim was that "const means thread safe". Both const
and non-const are thread-safe if you use RW locks or mutexes: you can
just as well read objects concurrently under a RW lock which are held by
non-const reference as those held by const reference. Constness
doesn't come into it.

There is a category error being made here: see below.

> > One thread may reference an object by const reference and another
> > thread may at the same time reference the same object by non-const
> > reference; this may happen merely as a consequence of function
> > signatures. The thread referencing the object by const reference,
> > say as a function argument, can make no automatic assumptions about
> > whether it can safely read that object without synchronization.
>
> I understood that Herb Sutter meant such design style. It can't be
> that he literally meant that we get thread safety for free by using
> const keyword and synchronization is not needed with such.
>
> For example of design style one can make it so that object does not
> return non-const references to its members. It is achievable merely by
> not using such function signatures in interface of class. As result
> non-const references to its members do not spread all around the board
> and so every by-passer can't modify those members.
>
> > When Herb said "const means thread-safe" he was not referring to
> > objects which are _created_ as const objects and so which cannot
> > lawfully be modified at all, if that was your point.
>
> It wasn't my point. My point was that communication between threads
> has to be under control of design like communication between objects
> has to be under control of design.
>
> Now lets start from what I think has to be hurtingly obvious. If
> something passes const references to some object to other thread
> then that sure has to mean promise to not destroy and deallocate
> that object when the other thread is processing it. Or how else?

Except that that is wrong. It is perfectly OK (as far as the C++
standard is concerned) to call the delete expression on an object via a
reference to const, just as you can call the delete expression on a
pointer to const. Constness is about whether a particular section of
code modifies an object's state, not about whether it deletes the
object. Some people say that that is a hole in the language. But when
you claim that const means thread-safe, you take the language as it is,
not as you think it should be.

I agree with very little of this.

Thread safety is about shared state: if there is no shared state,
thread safety is irrelevant. You say "in real software majority of
objects are actually immutable". The sad fact is that in real
software, most shared state is mutable, otherwise sharing it is barely
relevant. You are right, as I previously said, that in those few cases
where the shared state itself is immutable, then in those cases (and
only in those cases) the immutability does confer thread safety, at
least so far as the standard library is concerned.

The problem is that trying to argue that const means thread safe is a
category error. When you say that something is held as const it means
only that the particular piece of code holding the object as const may
not modify it. Thread safety however is about every thread to which the
object is visible in some form: it is about what the whole program can
do at any point in time, not about what one particular thread can do at
any point in time.

Chris

[Öö Tiib]

On Friday, 13 October 2017 12:05:23 UTC+3, Chris Vine wrote:
> On Thu, 12 Oct 2017 22:40:02 -0700 (PDT)
> Öö Tiib <oot...@hot.ee> wrote:

snip

> > Now lets start from what I think has to be hurtingly obvious. If
> > something passes const references to some object to other thread
> > then that sure has to mean promise to not destroy and deallocate
> > that object when the other thread is processing it. Or how else?
>
> Except that that is wrong. It is perfectly OK (as far as the C++
> standard is concerned) to call the delete expression on an object via a
> reference to const, just as you can call the delete expression on a
> pointer to const.

Of course I can take address of reference to automatic storage object
and call delete to it and I can also call const_cast and write to the
const object and same way I can easily remove all effects of private
by just few lines of code.
Unlike with actual shooting into my actual feet (where I may become
prosecuted for attempt of suicide by some jurisdictions) it is all legal.

> Constness is about whether a particular section of
> code modifies an object's state, not about whether it deletes the
> object. Some people say that that is a hole in the language. But when
> you claim that const means thread-safe, you take the language as it is,
> not as you think it should be.

And here is likely the error in your thinking. C++ is full anarchy language,
do whatever you want, all soft checks that compiler does can be ignored
and rest of the things can be worked around. Or can you bring any
examples of opposite? That was also from list of painfully obvious things
to anyone exposed to C++.
So I meant that the programmer who wrote that passer that passes a
reference to const to other thread has to be author of that promise.
Otherwise the software contains defect.

>
> > Further it also means promise not to modify that object anymore
> > or it means promise that the object synchronizes accesses of it
> > itself (for example by using atomics). That is also obvious?
> >
> > If passer can't make such promises then it should pass copies of
> > object (instead of references) to that other thread. Period.
> > That is how I understood Herb Sutter.

Lack of comments here indicate that you did read it, understood your
error but decided to whine on about how bad is C++ and Herb Sutter.
Use other languages then.

> >
> > > He was
> > > interpreting the provision in the standard which provides that in
> > > the absence of concurrent mutation, a type in the standard library
> > > can safely be read concurrently by two (or more) threads without
> > > further synchronization, and extrapolating from that; and he was
> > > referring to objects which are held by const reference or to which
> > > const methods are applied.
> > >
> > > As I said this was a highly misleading summary. Constness is
> > > concerned with something different from thread safety: by itself it
> > > guarantees nothing about thread safety, unless the object concerned
> > > happened to be created as a const object.
> >
> > It must be was put up in unfortunate and misleading manner because
> > as I understand it ... you were mislead.
> >
> > In real software majority of objects are actually immutable (may have
> > short initialization immediately after their creation). Passing not
> > synchronized const references to such objects between threads is safe
> > and easy and works.
> >
> > However here is also a trap. Software will evolve and the immutable
> > things become "mostly immutable but sometimes configurable". Result
> > is potential race condition during that "configuration".

Lack of comments here indicate that you did read it, understood your
error but decided to whine on about how bad is C++ and Herb Sutter.
Use other languages then.

And I also wrote that if author of your crap passer of reference to
constant can not promise that it (the described crap itself) won't
destroy or modify the object while other thread (to what it passed the
thing) is potentially using it in processing then it should pass copy of
object and not const reference to it. It is still quoted above.

> The problem is that trying to argue that const means thread safe is a
> category error. When you say that something is held as const it means
> only that the particular piece of code holding the object as const may
> not modify it. Thread safety however is about every thread to which the
> object is visible in some form: it is about what the whole program can
> do at any point in time, not about what one particular thread can do at
> any point in time.

The problem is that you misunderstood what was told to you and pulled
pile of awful and painfully obviously awful nonsense out of it. my current
experience how you pretend that you do not understand what is told to
you makes it close to certain.

[Chris Vine]

On Fri, 13 Oct 2017 03:14:45 -0700 (PDT)
Öö Tiib <oot...@hot.ee> wrote:
[snip]

> Lack of comments here indicate that you did read it, understood your
> error but decided to whine on about how bad is C++ and Herb Sutter.
> Use other languages then.

[snip]

> The problem is that you misunderstood what was told to you and pulled
> pile of awful and painfully obviously awful nonsense out of it. my
> current experience how you pretend that you do not understand what is
> told to you makes it close to certain.

Your posting is the most bizarre piece of writing I have ever seen. It
is completely obvious that I was making no comment "about how bad is
C++". Nor was I commenting on Herb Sutter except in relation to the
choice of his headline summary that "const means thread safe" in the
context of online videos, which was itself in the context of a posting
by someone else which (correctly) pointed out that if you have to lock
threads which modify shared state, you also need to lock threads which
read it.

I don't want to prolong this weird ranting, but on the first paragraph
above I tried not to comment on barely intelligible parts of your
posting which were irrelevant to what had appeared to become the issue.

[Öö Tiib]

Why do not you complain that "resource allocation is initialization" is also
"bad summary", (this time from Stroustrup), since resource allocation does
initialize nothing and strictly from C++ standard one may even call new
without assigning its outcome to anything? If you do not understand what
is told to you then my current impression is that the issue is most likely
on your side.

[Stuart Redmann]

That's quite a clue! Apparently there is a bug in your implementation of
the comparator object which has been found in debug mode through additional
checks in your STL implementation. In release mode these checks will not
get executed and your app is simply crashing.

Note that you may get breaks in your code even if you have set no
breakpoints. The easiest way to make an application break is to insert
__asm { int 4; } into your code. Under windows that is the equivalent of a
breakpoint in your IDE (if you set a breakpoint in the IDE, the IDE will
simply replace the processor instruction of the breakpoint line by the
interrupt 4 command, which is the software IRQ for breakpoints on Intel
machines).

This debug window should give you enough information about where the error
is: std::less<MyClass>!

Regards,
Stuart

[Christopher Pisz]

Indeed, in the end, it was the comparator. 5 days of searching. 4 people looked over the comparator and none saw the problem.

Let this be a lesson learned. Unit Test that (A < B) && (B < A) should never be true.

[Jorgen Grahn]

On Tue, 2017-10-17, Christopher Pisz wrote:
> On Friday, October 13, 2017 at 1:30:27 PM UTC-5, Stuart Redmann wrote:

...

>> That's quite a clue! Apparently there is a bug in your implementation of
>> the comparator object which has been found in debug mode through additional
>> checks in your STL implementation. In release mode these checks will not
>> get executed and your app is simply crashing.

> Indeed, in the end, it was the comparator. 5 days of searching. 4
> people looked over the comparator and none saw the problem.
>
> Let this be a lesson learned. Unit Test that (A < B) && (B < A)
> should never be true.

Interesting! I would never have imagined (and have never seen) that a
flawed T < T could cause a crash, but it's arguably better than
silently producing incorrect results.

You'd have to be careful to produce a reasonably complete unit test
though ... probably as careful as someone writing a correct T < T
in the first place.