Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make GILReleaser exception-safe #2363

Merged
merged 1 commit into from
Oct 13, 2020

Conversation

gleichdick
Copy link
Contributor

Description

If an exception occurs while the GIL is released,
it won't be reacquired and the stack unwinding will take place
without the GIL. Releasing and Reacquiring wil be removed.

For example,

bp::object wrap_long_computation(bp::object arg) {
   auto parsed_arg = parse_arg(arg);
   GILReleaser gr;
   auto res1 = do_long_computation_1(parsed_arg);
   gr.reacquire();
   bp::object res2 = do_more_python_stuff();
   gr.release();
   Result3 res3 = do_long_computation_2(res1);
   gr.reacquire();
   return encode_result(res2, res3);
}

is not exception safe, because if do_long_computation_2() throws an exception, the Python object res2 will be destroyed without the GIL.
Instead,

bp::object wrap_long_computation(bp::object arg) {
   auto parsed_arg = parse_arg(arg);
   Result1 res1;
   {
      GILReleaser gr;
      res1 = do_long_computation_1(parsed_arg);
   }
   bp::object res2 = do_more_python_stuff();
   Result3 res3;
   {
      GILReleaser gr;
      res3 = do_long_computation_2(res1);
   }
   return encode_result(res2, res3);
}

will be safe.

An IIFE would also work

...
  Result3 res3 = [&](){
     GILReleaser gr;
     return do_long_computation_2(res1);
  }();
...

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extend the tutorials / documentation reference
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests, which fail without this PR reference
  • Include a screenshot if changing a GUI
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

If an exception occurs while the GIL is released,
it won't be reacquired and the stack unwinding will take place
without the GIL. Releasing and Reacquiring wil be removed.
@codecov
Copy link

codecov bot commented Oct 10, 2020

Codecov Report

Merging #2363 into master will decrease coverage by 0.01%.
The diff coverage is 21.06%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2363      +/-   ##
==========================================
- Coverage   56.35%   56.34%   -0.00%     
==========================================
  Files         285      285              
  Lines       24242    24242              
==========================================
- Hits        13658    13657       -1     
- Misses      10584    10585       +1     
Impacted Files Coverage Δ
...ove_group_interface/src/wrap_python_move_group.cpp 41.72% <6.25%> (ø)
...ls/include/moveit/py_bindings_tools/gil_releaser.h 100.00% <100.00%> (ø)
...ipulation/pick_place/src/manipulation_pipeline.cpp 72.35% <0.00%> (-1.06%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5fe04a7...127e23e. Read the comment docs.

Copy link
Contributor

@v4hn v4hn left a comment

Choose a reason for hiding this comment

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

Thank you for the detailed analysis and the patch! 🏅

Both make sense to me and should be merged soon.

@v4hn v4hn added the awaits 2nd review one maintainer approved this request label Oct 12, 2020
Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

@rhaschke rhaschke merged commit c43b588 into moveit:master Oct 13, 2020
rhaschke pushed a commit that referenced this pull request Oct 13, 2020
If an exception occurs while the GIL is released, it won't be reacquired and the stack unwinding will take place
without the GIL. GILReleaser::release() and GILReleaser::reacquire() is removed, because it is unsafe.
pull bot pushed a commit to shadow-robot/moveit that referenced this pull request Oct 13, 2020
If an exception occurs while the GIL is released, it won't be reacquired and the stack unwinding will take place
without the GIL. GILReleaser::release() and GILReleaser::reacquire() is removed, because it is unsafe.
@tylerjw tylerjw mentioned this pull request Oct 23, 2020
57 tasks
@gleichdick gleichdick deleted the exception_safe_gilreleaser branch January 6, 2021 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaits 2nd review one maintainer approved this request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants