Skip to content

Conversation

@elkhrt
Copy link
Contributor

@elkhrt elkhrt commented Jan 25, 2021

…ak_ptr instead of a try/catch block.

Description

EDIT (@YannickJadoul): Quoting #2819 (comment)

My motivation for changing this is primarily a Google-internal one. Our C++ code is generally compiled with exceptions disabled - throwing an exception results in SIGABRT. For most uses of exceptions in pybind11, this is not an issue because exceptions are localized to Python interface code, and so we can compile that single module with exceptions enabled. But this particular bad_weak_ptr exception would be thrown by code in libc and so we can't rely on that having been built with exceptions enabled. Hence our need for avoiding the exception in 'normal' code paths.

Suggested changelog entry:

Avoid relying on exceptions in C++17 when getting a ``shared_ptr`` holder from a ``shared_from_this`` class.

YannickJadoul
YannickJadoul previously approved these changes Jan 25, 2021
Copy link
Collaborator

@YannickJadoul YannickJadoul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks good to me!

Seems like this isn't even an improvement to avoid try-catch, but even a fix that avoids UB on C++<17?

Quoting https://en.cppreference.com/w/cpp/memory/enable_shared_from_this/shared_from_this

It is permitted to call shared_from_this only on a previously shared object, i.e. on an object managed by std::shared_ptr (in particular, shared_from_this cannot be called during construction of *this).

Otherwise the behavior is undefined (until C++17)std::bad_weak_ptr is thrown (by the shared_ptr constructor from a default-constructed weak_this) (since C++17).

There's no such note for weak_from_this: https://en.cppreference.com/w/cpp/memory/enable_shared_from_this/weak_from_this

Do you happen to know if this is now completely safe?

@YannickJadoul YannickJadoul added this to the v2.6.2 milestone Jan 25, 2021
@elkhrt
Copy link
Contributor Author

elkhrt commented Jan 25, 2021

Sadly weak_from_this only exists from C++ 17 onwards, so should probably revert to old behaviour for older versions of C++, undefined behaviour and all. I'll amend the PR.

@YannickJadoul YannickJadoul self-requested a review January 25, 2021 15:26
@YannickJadoul YannickJadoul dismissed their stale review January 25, 2021 15:26

Extra changes

@YannickJadoul YannickJadoul modified the milestones: v2.6.2, v2.7 Jan 25, 2021
Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR was run through the Google global testing (by the author) and is already in the production environment.

@elkhrt
Copy link
Contributor Author

elkhrt commented Jan 30, 2021

My attempts to add Microsoft-specific checks seem to have failed, for reasons which are obscure to me.

@YannickJadoul
Copy link
Collaborator

The one failed test is not to worry about; it's one we sometimes see and hasn't been fixed yet. Unrelated to this PR.

@YannickJadoul YannickJadoul force-pushed the exception_free_shared_ptr branch from ac9dd31 to 651fd41 Compare January 30, 2021 13:16
@YannickJadoul
Copy link
Collaborator

Refactored out the function (so we only need to write this condition once) and added the <version> that @bstaletic mentioned. I believe this is what you meant, @bstaletic?

One more thing, @elkhrt: what was your reason for this change? According to cppreference, we still have UB in C++ versions < C++17, and the try-catch was already non-UB in C++17 and later. So I'm not sure what's the advantage that justifies partially duplicating this code?
Then again, it's only a handful of lines extra, so I'm fine including it. It's just that the refactoring shows we're actually not getting anything out of this extra complexity?

Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks @YannickJadoul!

Copy link
Collaborator

@YannickJadoul YannickJadoul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @rwgk. Pushed another commit with this fixed. One more question to resolve. If it's up to me, I'd still simplify the code (and ironically +- end up where we started).

@YannickJadoul
Copy link
Collaborator

Green, green, green, green, ...

Thanks, @elkhrt!

@YannickJadoul YannickJadoul merged commit 23c3edc into pybind:master Jan 30, 2021
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Jan 30, 2021
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Jul 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants