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

pythonbuf fix #2675

Merged
merged 7 commits into from
Nov 19, 2020
Merged

pythonbuf fix #2675

merged 7 commits into from
Nov 19, 2020

Conversation

nickbridgechess
Copy link
Contributor

Description

Fixes segfaults in multithreaded environments when using scoped_ostream_redirect.

The cause turns out to be the construction of a python str outside the GIL.

Recreate added to tests.

Suggested changelog entry:

Fixes segfaults in multithreaded environments when using scoped_ostream_redirect.

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.

Great catch! Thanks!

Is this related to this weird occasional failure on Windows, perhaps, @henryiii? I can't immediately find the issue (or maybe there is none), but it's hinting into that direction.

tests/CMakeLists.txt Outdated Show resolved Hide resolved
tests/CMakeLists.txt Outdated Show resolved Hide resolved
tests/test_thread.cpp Outdated Show resolved Hide resolved
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.

A few things, but nothing drastic, so I'm fine with not having these done, as long as CI passes.

tests/test_iostream.cpp Outdated Show resolved Hide resolved
tests/test_iostream.cpp Outdated Show resolved Hide resolved
include/pybind11/iostream.h Outdated Show resolved Hide resolved
tests/test_iostream.cpp Outdated Show resolved Hide resolved
@henryiii henryiii added this to the v2.6.2 milestone Nov 18, 2020
@nickbridgechess
Copy link
Contributor Author

Any idea why some of the checks are failing? MSCV, for example, is complaining about unreachable code, and no object file being created... This is beyond me.

@YannickJadoul
Copy link
Collaborator

Any idea why some of the checks are failing? MSCV, for example, is complaining about unreachable code, and no object file being created... This is beyond me.

I don't think you're to blame, to be honest :-| Could this just be MSVC/VS 2015 tripping over it's own stdlib headers?
#77

@nickbridgechess
Copy link
Contributor Author

Thanks for all your help on this. Is there anything else I need to do?

@YannickJadoul
Copy link
Collaborator

Thanks for all your help on this. Is there anything else I need to do?

I'm looking how to shut up MSVC. If you don't mind any of us pushing changes to your branch, then nope :-)

@nickbridgechess
Copy link
Contributor Author

Thanks for all your help on this. Is there anything else I need to do?

I'm looking how to shut up MSVC. If you don't mind any of us pushing changes to your branch, then nope :-)

Do whatever you have to! LMK if you need anything from me.

@YannickJadoul
Copy link
Collaborator

Do whatever you have to! LMK if you need anything from me.

Great! Thanks a lot for the quick fixes, and for finding this in the first place! :-)

@YannickJadoul YannickJadoul force-pushed the pythonbuf_fix branch 2 times, most recently from 9928fc4 to 9e81c2d Compare November 18, 2020 22:10
@YannickJadoul
Copy link
Collaborator

Windows tests just failed with the known fluke:

  _____________________________ test_multi_captured _____________________________
  
  capfd = <_pytest.capture.CaptureFixture object at 0x0000017DFE653508>
  
      def test_multi_captured(capfd):
          stream = StringIO()
          with redirect_stdout(stream):
              m.captured_output("a")
              m.raw_output("b")
              m.captured_output("c")
              m.raw_output("d")
          stdout, stderr = capfd.readouterr()
  >       assert stdout == "bd"
  E       AssertionError: assert '' == 'bd'
  E         - bd
  
  test_iostream.py:152: AssertionError
  ________________________________ test_redirect ________________________________
  
  capfd = <_pytest.capture.CaptureFixture object at 0x0000017DFE33F1C8>
  
      def test_redirect(capfd):
          msg = "Should not be in log!"
          stream = StringIO()
          with redirect_stdout(stream):
              m.raw_output(msg)
          stdout, stderr = capfd.readouterr()
  >       assert stdout == msg
  E       AssertionError: assert '' == 'Should not be in log!'
  E         - Should not be in log!
  
  test_iostream.py:169: AssertionError
  ______________________________ test_redirect_err ______________________________
  
  capfd = <_pytest.capture.CaptureFixture object at 0x0000017DFDF09108>
  
      def test_redirect_err(capfd):
          msg = "StdOut"
          msg2 = "StdErr"
      
          stream = StringIO()
          with redirect_stderr(stream):
              with m.ostream_redirect(stdout=False):
                  m.raw_output(msg)
                  m.raw_err(msg2)
          stdout, stderr = capfd.readouterr()
  >       assert stdout == msg
  E       AssertionError: assert '' == 'StdOut'
  E         - StdOut
  
  test_iostream.py:198: AssertionError

So this PR unfortunately does not fix them :-(

@YannickJadoul
Copy link
Collaborator

Finally, green! Good to merge, as far as I'm concerned! :-)

@YannickJadoul YannickJadoul merged commit 2fa4747 into pybind:master Nov 19, 2020
@YannickJadoul
Copy link
Collaborator

Thanks, @nickbridgechess! :-)

@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Nov 19, 2020
@nickbridgechess
Copy link
Contributor Author

You're very welcome. I appreciate the help, and getting this fix in so quickly.
Great project - very useful and well written+designed.

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.

None yet

5 participants