"peer reviewed" - Rights and responsibilities of maintainers

classic Classic list List threaded Threaded
58 messages Options
123
Reply | Threaded
Open this post in threaded view
|

"peer reviewed" - Rights and responsibilities of maintainers

Boost - Dev mailing list
I'm having trouble with a maintainer of a boost library over a PR and am
seeking for clarification.

Background (may be skipped):
A bug was introduced into the library which I noticed in one of "my"
(company) projects leading to a reproducible crash. However the scenario
is not that common (although others have the same problem) so it wasn't
noticed earlier.
I analyzed the problem and created a minimum working example to
reproduce this and then opened a PR with a fix. The maintainer did not
comment on this for multiple months so given it IS complicated, I
factored out additional PRs with tests I created to show the problem, so
each PR is fairly small. In the result there were PRs with tests that
failed on travis and the fix-PR which included the test-PRs and the fix
and succeeded on travis (and all other CIs) and users reported success
using this PR.
Still no response from the maintainer on any PR. He then added an own
test claiming to be enough, while I (as the author of the original
tests) knew it wasn't as e.g. it didn't fail. (so what's it worth then?)
Furthermore he added a commit changing unrelated stuff and ADDITIONALLY
changing code I touched in the fix claiming that it would be the essence
of my patch and fix the problems. Again I (as the author of the original
patch) know that this is not true especially as the test-PRs still fail
with his patch applied.


So my point/problem is: Is the maintainer allowed to simply do changes
as he wishes without any review at all?

My expected procedure is:
- Someone creates a PR
- Maintainer (and optionally the community) reviews (and comments on) it
- It eventually gets merged or rejected with comments why
OR:
- Someones else (including the maintainer) creates a contra-PR fixing
the same problem
- That author explains, how and why this fixes the same problem and why
it is superior to the original PR
- This gets reviewed and either PR gets merged

Arguments were made, that this doesn't apply to accepted boost libraries
as e.g. "it's [the maintainers] heads that got chopped off if something
gets broken".
I would challenge this by the simple example at hand: This boost library
IS broken and it crashes MY application (and others). But I see no heads
rolling.

Furthermore: If the maintainer is allowed to patch and change as he
wants without ANY review, how can Boost still be called "peer reviewed"?
My trust in Boost stems from the extensive review a library has to
undergo before it is included in Boost. If afterwards no review is
happening or reviews are actively avoided (as in the current example),
then the older a Boost library gets, the less trust I would have in it
as more and more unreviewed changes get included.

So again: What is the Boost course on reviews of existing libraries and
the statement on the rights and responsibilities of maintainers?

Thanks,
Alexander Grund



_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

smime.p7s (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: "peer reviewed" - Rights and responsibilities of maintainers

Boost - Dev mailing list
On 10/16/18 9:59 AM, Alexander Grund via Boost wrote:

> So again: What is the Boost course on reviews of existing libraries and
> the statement on the rights and responsibilities of maintainers?

https://www.boost.org/community/reviews.html#Maintainer

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: "peer reviewed" - Rights and responsibilities of maintainers

Boost - Dev mailing list

>
>> So again: What is the Boost course on reviews of existing libraries
>> and the statement on the rights and responsibilities of maintainers?
>
> https://www.boost.org/community/reviews.html#Maintainer
Thanks.
"You are free to change your library in any way you wish". Ok...

BTW: Should this paragraph possibly get an update to refer to Github
(PRs, issues, ...) and how to handle patches from others?
It says "peer review is an important part of the Boost process" but only
relates to interface changes, not to internal changes although they
might break things or are supposed to fix them. "and to participate as
needed in discussions of your library on the boost mailing lists." could
be extended to include Github(?)



_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

smime.p7s (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: "peer reviewed" - Rights and responsibilities of maintainers

Boost - Dev mailing list
On 10/16/18 7:01 AM, Alexander Grund via Boost wrote:

>
>>
>>> So again: What is the Boost course on reviews of existing libraries
>>> and the statement on the rights and responsibilities of maintainers?
>>
>> https://www.boost.org/community/reviews.html#Maintainer
> Thanks.
> "You are free to change your library in any way you wish". Ok...
>
> BTW: Should this paragraph possibly get an update to refer to Github
> (PRs, issues, ...) and how to handle patches from others?
> It says "peer review is an important part of the Boost process" but
> only relates to interface changes, not to internal changes although
> they might break things or are supposed to fix them. "and to
> participate as needed in discussions of your library on the boost
> mailing lists." could be extended to include Github(?)
>

As someone trying to work my way up to take over as the graph
maintainer, having a maintainer do this rubs me the wrong way.  If this
kind of excitement is going to be stirred up, can you tell us the
subproject so we can figure out what is happening?


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: "peer reviewed" - Rights and responsibilities of maintainers

Boost - Dev mailing list

>> "You are free to change your library in any way you wish". Ok...
>>
>> BTW: Should this paragraph possibly get an update to refer to Github
>> (PRs, issues, ...) and how to handle patches from others?
>> It says "peer review is an important part of the Boost process" but
>> only relates to interface changes, not to internal changes although
>> they might break things or are supposed to fix them. "and to
>> participate as needed in discussions of your library on the boost
>> mailing lists." could be extended to include Github(?)
>>
>
> As someone trying to work my way up to take over as the graph
> maintainer, having a maintainer do this rubs me the wrong way.  If
> this kind of excitement is going to be stirred up, can you tell us the
> subproject so we can figure out what is happening?
I'd rather not, the discussion is already heated enough there I feel.
I just wanted to know where Boost (or other members/maintainers/...)
stand in this to reevaluate my expectations. Being on the other site
(contributor, not maintainer) it puzzled me, that maintainers can simply
ignore changes and actively avoid reviews when doing own changes.

Note that CMT maintained libs are much more like I thought it would
work: https://svn.boost.org/trac10/wiki/CommunityMaintenance
- Changes proposed
- Changes reviewed
- Changes merged



_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

smime.p7s (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: "peer reviewed" - Rights and responsibilities of maintainers

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On 10/16/18 2:55 AM, Bjorn Reese via Boost wrote:

> On 10/16/18 9:59 AM, Alexander Grund via Boost wrote:
>
>> So again: What is the Boost course on reviews of existing libraries
>> and the statement on the rights and responsibilities of maintainers?
>
> https://www.boost.org/community/reviews.html#Maintainer
>
> _______________________________________________
> Unsubscribe & other changes:
> http://lists.boost.org/mailman/listinfo.cgi/boost
>

Just to clarify I'm am the maintainer in question.

Basically I don't believe that Alexanders PRs are correct and I haven't
found his tests to demonstrate the contrary convincing.

Anyone who is interested can check the history of the issue in github.

Robert Ramey


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: "peer reviewed" - Rights and responsibilities of maintainers

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
Alexander Grund wrote:

> So my point/problem is: Is the maintainer allowed to simply do changes as
> he wishes without any review at all?

In short, yes. I understand your frustration, but the procedure is to square
it off with the maintainer, then if you feel you're getting nowhere,
escalate to the list. Basically what you've done, except that you're trying
to discuss not the issue at hand, but "rights and responsibilities of
maintainers", which is not going to help your issue, or anyone.

The test in question is

https://github.com/boostorg/serialization/pull/111/files

which does indeed crash

https://travis-ci.org/boostorg/serialization/jobs/441654483#L2546

That's easy enough to follow and does show a legitimate problem, in my
opinion, although Robert refuses to acknowledge it for some reason.

Your further PRs, #110 and #105, are more convoluted and I haven't had the
time to look into them in detail; the "fix" PR was failing on Travis, so I
assumed it was still a work in progress. Looking at it now,

https://travis-ci.org/boostorg/serialization/builds/442032113

the only failures are with Clang and they happen without your patch as well.

In either case, it has to be Robert who approves and applies the patch, as
"the community" doesn't understand the library as well as he does. (I
certainly do not.)


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: "peer reviewed" - Rights and responsibilities of maintainers

Boost - Dev mailing list
On 10/16/18 7:23 AM, Peter Dimov via Boost wrote:

> Alexander Grund wrote:
> >
> https://github.com/boostorg/serialization/pull/111/files
>
> which does indeed crash
>
> https://travis-ci.org/boostorg/serialization/jobs/441654483#L2546
>
> That's easy enough to follow and does show a legitimate problem, in my
> opinion, although Robert refuses to acknowledge it for some reason.

The test does not show anything at all. It's ill conceived.  A similar
test could be made and it might be useful, but if I do all I'll get is a
lot of heat from people who haven't take the time to understand what is
actually going on.

> the only failures are with Clang and they happen without your patch as
> well.

My version of the patch captures the essence of the PR while retaining
the original intent of the code.  It's much simpler and alters many less
files. I have no idea why I am being criticized for making this patch.

> In either case, it has to be Robert who approves and applies the patch,
> as "the community" doesn't understand the library as well as he does. (I
> certainly do not.)

FWIW - I often forget how the library works.  Sometimes an issue comes
up with code that hasn't been visited in over a decade.  In such cases I
have to review the documentation, tests and source code.  I think I'm
the only one who actually does this.

Robert Ramey


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: "peer reviewed" - Rights and responsibilities of maintainers

Boost - Dev mailing list
Robert Ramey wrote:

> On 10/16/18 7:23 AM, Peter Dimov via Boost wrote:
>
> > https://github.com/boostorg/serialization/pull/111/files
> >
> > which does indeed crash
> >
> > https://travis-ci.org/boostorg/serialization/jobs/441654483#L2546
> >
> > That's easy enough to follow and does show a legitimate problem, in my
> > opinion, although Robert refuses to acknowledge it for some reason.
>
> The test does not show anything at all. It's ill conceived.

What is ill-conceived about it? It's a minimal example of two shared
libraries each using Serialization. As Alexander says, it's a simplified
version of a crash they had in one of their projects.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: "peer reviewed" - Rights and responsibilities of maintainers

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list

> In short, yes. I understand your frustration, but the procedure is to
> square it off with the maintainer, then if you feel you're getting
> nowhere, escalate to the list. Basically what you've done, except that
> you're trying to discuss not the issue at hand, but "rights and
> responsibilities of maintainers", which is not going to help your
> issue, or anyone.
I already escalated the issue to the list to avoid getting the crashing
version into the last release. This obviously failed.
My post here was after in the discussion of/in PR I got the impression
that I'm expecting to much of the maintainer: A review of my changes, I
developed, tested and submitted. Someones else agreed to my point of
view while others opposed. Hence I wanted clarification on the meta
issue with a reference which I could bring on. Seems like the facts are
against me in this case so I (or my changes) are in the mercy of the
maintainer.

As you mentioned it:
#105 adds more CI
#110 includes #105 and tests that singletons are eventually destructed
and `is_destroyed` returns true in both use cases of the singleton
#111 Is the condensed version of my MWE that shows the crash. Essence:
Use 2 shared libraries linked against static boost and each uses a part
of Boost.Serialization. The crash comes from a destruction order
problem, that is currently unsolved and could be avoided if
`is_destroyed` would return true, but it doesn't

But Robert insists that #111 "does not show anything at all. It's ill
conceived." He does not acknowledge that Boost.Serialization makes the
application crash and does not bring any argument why it would be "ill
conceived". I do not understand this. It obviously crashes. This is a
fact easily reproducible by anyone on linux. If the test misuses C++
(UB, ...) or the library, then where and how? If not, why is it seen as
"ill conceived"?

 > My version of the patch captures the essence of the PR while
retaining the original intent of the code.

This is simply wrong. If it would #110 and #111 would pass. It fails to
fix the `is_destroyed` function or the core problem with destruction order

 > It's much simpler and alters many less files.

My fix alters 1 file to fix `is_destroyed` and 4 more to remove a single
line with an assertion which doesn't hold as shown in #111. Most of the
changes are added reverts to an earlier, working version and comments on
why and how stuff works, so it doesn't get accidentally broken by
optimization/reduction efforts as happened in 1.65

 > I have no idea why I am being criticized for making this patch.

Because a) it combines unrelated changes, hiding the fix and b) pushing
it w/o review by the original patch author who might have reasonable
arguments why it isn't enough or "the essence of the PR". I learned it
is your right to do that. But then again it is my right to to tell you
that your claim is wrong. As I as the author of the patch might
understand the patch better, it is quite possible, that I'm right, isn't it?



_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

smime.p7s (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: "peer reviewed" - Rights and responsibilities of maintainers

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On 10/16/18 8:11 AM, Peter Dimov via Boost wrote:

> Robert Ramey wrote:
>> On 10/16/18 7:23 AM, Peter Dimov via Boost wrote:
>>
>> > https://github.com/boostorg/serialization/pull/111/files
>> >
>> > which does indeed crash
>> >
>> > https://travis-ci.org/boostorg/serialization/jobs/441654483#L2546
>> >
>> > That's easy enough to follow and does show a legitimate problem, in
>> my > opinion, although Robert refuses to acknowledge it for some reason.
>>
>> The test does not show anything at all. It's ill conceived.
>
> What is ill-conceived about it? It's a minimal example of two shared
> libraries each using Serialization. As Alexander says, it's a simplified
> version of a crash they had in one of their projects.

First of all, the test changes every time I look at it.  It's hard to
follow.

On the last version I looked at - since no there is not
BOOST_CLASS_EXPORT(class name) invoked, there are no indexed entries in
the extended type info tables so get_key() should return null under any
circumstances.  It tests nothing.  Worse, it betrays the fact that the
author of the test, and anyone who doesn't see a problem with it, have
read neither the document nor the source code. Rather than asking
"What's ill conceived about it" better to ask - "What is this supposed
to be testing?"

Robert Ramey


>
> _______________________________________________
> Unsubscribe & other changes:
> http://lists.boost.org/mailman/listinfo.cgi/boost
>



_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: "peer reviewed" - Rights and responsibilities of maintainers

Boost - Dev mailing list
Robert Ramey wrote:

> On the last version I looked at - since no there is not
> BOOST_CLASS_EXPORT(class name) invoked, ...

There are no class names in the test; it uses int and float.

> Rather than asking "What's ill conceived about it" better to ask - "What
> is this supposed to be testing?"

The use case of two shared libraries each using Serialization, as already
explained.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: "peer reviewed" - Rights and responsibilities of maintainers

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
First: This would have been the thing I was looking for in the MR.
> On the last version I looked at - since no there is not
> BOOST_CLASS_EXPORT(class name) invoked, there are no indexed entries
> in the extended type info tables so get_key() should return null under
> any circumstances.
Ok, but this is defined, correct? It is not undefined behaviour or
anything else.
Besides: And it is using standard types: "int" and "float", no classes.
So what export is missing there?
> It tests nothing.
It tests the construction and destruction of Boost.Serialization library
types in a specific context.||||
> Worse, it betrays the fact that the author of the test, and anyone who
> doesn't see a problem with it, have read neither the document nor the
> source code.
I was going for a MWE: Minimal code to show the problem.
https://www.boost.org/doc/libs/1_67_0/libs/serialization/doc/tutorial.html 
does NOT show, that BOOST_CLASS_EXPORT is required. I can change the
code to something like:
int i = 42;|
boost::archive::text_oarchive(std::cout) & i;

And
|
|float i = 42;|
boost::archive::text_oarchive(std::cout) & i;

||

||And it would still crash.||

||||

> Rather than asking "What's ill conceived about it" better to ask -
> "What is this supposed to be testing?"
 From the description: This adds a test (from #105
<https://github.com/boostorg/serialization/pull/105>) which uses
singletons in shared libraries linked against static boost. This crashes
on termination.
It simply **uses** singletons and crashes.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

smime.p7s (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

"peer reviewed" - Rights and responsibilities of maintainers

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
> -----Original Message-----
> From: Boost <[hidden email]> On Behalf Of Robert Ramey via Boost
> Sent: Tuesday, October 16, 2018 5:22 PM
>
> On 10/16/18 8:11 AM, Peter Dimov via Boost wrote:
> > Robert Ramey wrote:
> >> On 10/16/18 7:23 AM, Peter Dimov via Boost wrote:
> >>
> >> > https://github.com/boostorg/serialization/pull/111/files
> >> >
> >> > which does indeed crash
> >> >
> >> > https://travis-ci.org/boostorg/serialization/jobs/441654483#L2546
> >> >
> >> > That's easy enough to follow and does show a legitimate problem, in
> >> my > opinion, although Robert refuses to acknowledge it for some reason.
> >>
> >> The test does not show anything at all. It's ill conceived.
> >
> > What is ill-conceived about it? It's a minimal example of two shared
> > libraries each using Serialization. As Alexander says, it's a simplified
> > version of a crash they had in one of their projects.
>
> First of all, the test changes every time I look at it.  It's hard to
> follow.

If you keep saying that the test is bad / irrelevant, it is no wonder
the author changes the code to improve it.

>
> On the last version I looked at - since no there is not
> BOOST_CLASS_EXPORT(class name) invoked, there are no indexed entries in
> the extended type info tables so get_key() should return null under any
> circumstances.  

If I understand this correctly, the purpose of the test is to show that the
application crashes. In that case, the return value is completely irrelevant.

If you think that crash is not the fault of Boost.Serialization (e.g. because
it is used wrongly), then it would be helpful to explain what you believe to
be the actual reason for the crash.

What I don't know: Is there a travis build that shows that the test from Flamefire
(https://github.com/boostorg/serialization/pull/111/files ) still fails
with the changes introduced by Robert (I'm not sure when they were introduced)?

Mike


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: "peer reviewed" - Rights and responsibilities of maintainers

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On 10/16/18 8:32 AM, Peter Dimov via Boost wrote:

> Robert Ramey wrote:
>
>> On the last version I looked at - since no there is not
>> BOOST_CLASS_EXPORT(class name) invoked, ...
>
> There are no class names in the test; it uses int and float.
>
>> Rather than asking "What's ill conceived about it" better to ask -
>> "What is this supposed to be testing?"
>
> The use case of two shared libraries each using Serialization, as
> already explained.

Well it's not testing that.
>
> _______________________________________________
> Unsubscribe & other changes:
> http://lists.boost.org/mailman/listinfo.cgi/boost
>


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: "peer reviewed" - Rights and responsibilities of maintainers

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list

> If you keep saying that the test is bad / irrelevant, it is no wonder
> the author changes the code to improve it.
+1
And there have been no major changes. Just comments and return value
check changes as per PR comments.
>
> If I understand this correctly, the purpose of the test is to show that the
> application crashes. In that case, the return value is completely irrelevant.
+1
> What I don't know: Is there a travis build that shows that the test from Flamefire
> (https://github.com/boostorg/serialization/pull/111/files ) still fails
> with the changes introduced by Robert (I'm not sure when they were introduced)?
I rebased the changes to develop yesterday. So yes:
https://travis-ci.org/boostorg/serialization/jobs/441654483,
https://travis-ci.org/boostorg/serialization/jobs/441654485



_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

smime.p7s (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: "peer reviewed" - Rights and responsibilities of maintainers

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On 10/16/18 8:15 AM, Alexander Grund via Boost wrote:
>

> But Robert insists that #111 "does not show anything at all. It's ill
> conceived."
True
He does not acknowledge that Boost.Serialization makes the
> application crash
False
and does not bring any argument why it would be "ill
> conceived".
False
I do not understand this. It obviously crashes. This is a
> fact easily reproducible by anyone on linux.

If "It" refers to the test_dll_exported on clang - True
else False

If the test misuses C++
> (UB, ...) or the library, then where and how? If not,

If the test refers to your recent test - you test doesn't test anything.

If the test refers to the serialization library test_export_dll on clang
(with which version of standard library - who knows?) there is a
problem. That is why I made such a test after all.  So True

why is it seen as
> "ill conceived"?

It's based on a mis-understanding of how the serialization uses the
singleton and of how the singleton works.
>
>  > My version of the patch captures the essence of the PR while
> retaining the original intent of the code.
>
> This is simply wrong.

False

If it would #110 and #111 would pass.

False

It fails to
> fix the `is_destroyed` function or the core problem with destruction order

I see no evidence of this.
>
>  > It's much simpler and alters many less files.

Verifiably true by inspection.

>
> My fix alters 1 file to fix `is_destroyed` and 4 more to remove a single
> line with an assertion which doesn't hold as shown in #111. Most of the
> changes are added reverts to an earlier, working version and comments on
> why and how stuff works, so it doesn't get accidentally broken by
> optimization/reduction efforts as happened in 1.65
>
>  > I have no idea why I am being criticized for making this patch.
>
> Because a) it combines unrelated changes,

If a) refers to my patch -> Verifiably True by inspection

hiding the fix and b) pushing
> it w/o review by the original patch author who might have reasonable
> arguments why it isn't enough or "the essence of the PR". I learned it
> is your right to do that.

For good reason.

But then again it is my right to to tell you
> that your claim is wrong.

True.

As I as the author of the patch might
> understand the patch better, it is quite possible, that I'm right, isn't
> it?

It's possible - but after having considered that possibility, I've
concluded that it's False

Robert Ramey
>
>
>
>
> _______________________________________________
> Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
>



_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: "peer reviewed" - Rights and responsibilities of maintainers

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On 10/16/18 8:49 AM, Mike Dev via Boost wrote:

>> -----Original Message-----
>> From: Boost <[hidden email]> On Behalf Of Robert Ramey via Boost
>> Sent: Tuesday, October 16, 2018 5:22 PM
>>
>> On 10/16/18 8:11 AM, Peter Dimov via Boost wrote:
>>> Robert Ramey wrote:
>>>> On 10/16/18 7:23 AM, Peter Dimov via Boost wrote:
>>>>
>>>>> https://github.com/boostorg/serialization/pull/111/files
>>>>>
>>>>> which does indeed crash
>>>>>
>>>>> https://travis-ci.org/boostorg/serialization/jobs/441654483#L2546
>>>>>
>>>>> That's easy enough to follow and does show a legitimate problem, in
>>>> my > opinion, although Robert refuses to acknowledge it for some reason.
>>>>
>>>> The test does not show anything at all. It's ill conceived.
>>>
>>> What is ill-conceived about it? It's a minimal example of two shared
>>> libraries each using Serialization. As Alexander says, it's a simplified
>>> version of a crash they had in one of their projects.
>>
>> First of all, the test changes every time I look at it.  It's hard to
>> follow.
>
> If you keep saying that the test is bad / irrelevant, it is no wonder
> the author changes the code to improve it.

If the test is bad/irrelevant, what else should I say.  I have no
problem if the author want's to improve his test.  It's just not happening.

>
>>
>> On the last version I looked at - since no there is not
>> BOOST_CLASS_EXPORT(class name) invoked, there are no indexed entries in
>> the extended type info tables so get_key() should return null under any
>> circumstances.
>
> If I understand this correctly, the purpose of the test is to show that the
> application crashes. In that case, the return value is completely irrelevant.

I haven't seen the application so I don't know why it crashes.  I'm
guessing that it might be related to the current (and recently improved
state) singleton.  I DO know test_exported_dll also crashes on one
compiler configuration. I would like to fix this but so far no one has
been able to provide a convincing change nor a test which points out
where the problem might be.
>
> If you think that crash is not the fault of Boost.Serialization (e.g. because
> it is used wrongly), then it would be helpful to explain what you believe to
> be the actual reason for the crash.

Which crash: in his app, in test_exported_dll, or in his test?


>
> What I don't know: Is there a travis build that shows that the test from Flamefire
> (https://github.com/boostorg/serialization/pull/111/files ) still fails
> with the changes introduced by Robert (I'm not sure when they were introduced)?

Right - that are the exact same results which result from Alexander's
original patch.  That is, I merged in a simpler, less intrusive, less
fragile version of Alexander's patch.  And it "fixes" everything that
Alexander's patch fixed.  Clearly there are still some problems left -
which are likely to be even more difficult to nail down.  Note that to
get this far has required 9 months of effort - part of which entailed
including a breaking patch in to the release.

Under ordinary circumstances, I would just fix Alexander's test and be
done with it just to save time.  But I've come to understand that it
won't actually save time so I haven't done so.

Robert Ramey

>
> Mike
>
>
> _______________________________________________
> Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
>



_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: "peer reviewed" - Rights and responsibilities of maintainers

Boost - Dev mailing list
Robert Ramey wrote:

> Which crash: in his app, in test_exported_dll, or in his test?

His app crashes. His test, a simplified version of his app, crashes.

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: "peer reviewed" - Rights and responsibilities of maintainers

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On 17/10/2018 04:15, Alexander Grund wrote:
> As you mentioned it:
> #105 adds more CI
> #110 includes #105 and tests that singletons are eventually destructed
> and `is_destroyed` returns true in both use cases of the singleton
> #111 Is the condensed version of my MWE that shows the crash. Essence:
> Use 2 shared libraries linked against static boost and each uses a part
> of Boost.Serialization. The crash comes from a destruction order
> problem, that is currently unsolved and could be avoided if
> `is_destroyed` would return true, but it doesn't

Granted that I have not looked at the code or the PR in any detail (and
thus this might be entirely off base):

Conceptually "is_destroyed" is usually a bad idea.  By definition, after
the destructor has run the memory is free to get stomped on by other
objects and thus you can't rely on is_destroyed to return any particular
value.  Trying to rely on it is UB.

Applying this to singletons at global scope is both a blessing and a
curse -- it reduces the likelihood that something else would actually
stomp over the memory in practice, but also introduces a static
destruction order indeterminacy issue.

Usually the motivation to introduce this sort of thing is a side effect
of having dangling pointers to deleted objects.  In which case usually
the best solution is to recognise that you actually have a shared
ownership issue (where the object's actual lifetime is not what you
thought it was) and start using shared_ptr/weak_ptr and/or having an
object keep track of which singleton it is registered with rather than
assuming that it will find the same ambient singleton later.

Having said *that*, as previously discussed on the list, using a static
library from multiple shared libraries in itself introduces an aliasing
problem, where sometimes singletons aren't actually singletons -- and
woe betide you if you try to pass an object that references one
singleton across the library divide into the domain of the other
singleton, because then you can have issues such as registering with one
singleton and then trying to deregister from the other (which all by
itself can be a source of dangling pointers).

(FWIW, introducing dynamic libraries also increases the risk of dangling
pointers, if a library can be unloaded while references to it survive
outside.)

As far as I am aware (when you are using static-from-dynamic), you will
get these duplicated singletons at all times on Windows, and on Linux it
will happen whenever you are compiling with -fvisibility=hidden (which
appears to be true in this case given the build output) and if the
singleton definitions don't use BOOST_SYMBOL_VISIBLE.

I leave it to the rest of you to figure out whether this points at an
underlying issue in Boost.Serialization or whether it's a usage error.
Or both.

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
123