pdb: cls._pluginmanager, config etc might be None

[blueyed]

This can happen when debugging threads after pytest finished already (and the debugging plugin was unconfigured).

A failing test:

def test_pdb_in_thread_after_exit(testdir):
    """It should be imported in pytest_configure or later only."""
    p1 = testdir.makepyfile(
        """
        import threading


        def test():
            evt = threading.Event()
            evt2 = threading.Event()

            def target():
                evt.set()
                evt2.wait()
                print("target_set_trace")
                __import__('pdb').set_trace()
                print("target_end")

            thread = threading.Thread(target=target)
            thread.start()

            evt.wait()
            evt2.set()
        """,
    )
    child = testdir.spawn_pytest(str(p1) + " -s")
    child.expect("target_set_trace")
    rest = child.read().decode("utf8")
    assert "Exception in thread" not in rest
    assert child.exitstatus == 0

I think it should not pop the last set of attributes, but hard to say when the last time is maybe?!
IIRC it does not use pytest_unconfigure intentionally.

The test causes this:

Exception in thread Thread-1:
Traceback (most recent call last):
  File "/usr/lib/python3.7/threading.py", line 917, in _bootstrap_inner
    self.run()
  File "/usr/lib/python3.7/threading.py", line 865, in run
    self._target(*self._args, **self._kwargs)
  File "/tmp/pytest-of-user/pytest-61/test_pdb_in_thread_after_exit0/test_pdb_in_thread_after_exit.py", line 12, in target
    __import__('pdb').set_trace()
  File "…/Vcs/pytest/src/_pytest/debugging.py", line 221, in set_trace
    _pdb = cls._init_pdb(*args, **kwargs)
  File "…/Vcs/pytest/src/_pytest/debugging.py", line 212, in _init_pdb
    cls._pluginmanager.hook.pytest_enter_pdb(config=cls._config, pdb=_pdb)
AttributeError: 'NoneType' object has no attribute 'hook'

I've seen this initially:

(Pdb++[sync-to-async-_0]) c
Exception in worker
Traceback (most recent call last):
  File "/usr/lib/python3.7/concurrent/futures/thread.py", line 79, in _worker
    if work_item is not None:
  File "/usr/lib/python3.7/concurrent/futures/thread.py", line 79, in _worker
    if work_item is not None:
  File "/usr/lib/python3.7/bdb.py", line 88, in trace_dispatch
    return self.dispatch_line(frame)
  File "/usr/lib/python3.7/bdb.py", line 112, in dispatch_line
    self.user_line(frame)
  File "/usr/lib/python3.7/pdb.py", line 261, in user_line
    self.interaction(frame, None)
  File "…/project/.venv/lib/python3.7/site-packages/pdb.py", line 554, in interaction
    ret = self._interaction(frame, traceback)
  File "…/project/.venv/lib/python3.7/site-packages/pdb.py", line 599, in _interaction
    self._cmdloop()
  File "/usr/lib/python3.7/pdb.py", line 321, in _cmdloop
    self.cmdloop()
  File "…/project/.venv/lib/python3.7/site-packages/pdb.py", line 667, in cmdloop
    return self._inner_cmdloop(intro)
  File "…/project/.venv/lib/python3.7/site-packages/pdb.py", line 701, in _inner_cmdloop
    ret = super(Pdb, self).cmdloop(intro="\n".join(wrapped_intro))
  File "/usr/lib/python3.7/cmd.py", line 138, in cmdloop
    stop = self.onecmd(line)
  File "/usr/lib/python3.7/pdb.py", line 418, in onecmd
    return cmd.Cmd.onecmd(self, line)
  File "/usr/lib/python3.7/cmd.py", line 217, in onecmd
    return func(arg)
  File "…/Vcs/pytest/src/_pytest/debugging.py", line 159, in do_continue
    tw = _pytest.config.create_terminal_writer(cls._config)
  File "…/Vcs/pytest/src/_pytest/config/__init__.py", line 1085, in create_terminal_writer
    if config.option.color == "yes":
AttributeError: 'NoneType' object has no attribute 'option'

JFI: #5244 worked around this, by keeping pytest alive - which I am moving to my fork to close stale PRs on the main repo (blueyed#17).

[blueyed]

(not merged here, feel free to close it though again)

[blueyed]

JFI: can be fixed by not storing it on the class (blueyed#77, d7c3d9e).

[nicoddemus]

Sorry, why are you fixing it in your fork, instead of on the main repository? 🤔

[nicoddemus]

Oh I see: #5244

Well it is a shame the discussion just lost traction there.

[blueyed]

Sorry, why are you fixing it in your fork, instead of on the main repository?

Described in the commit: d7c3d9e (I've tried to rebase it on features, but there were two minor other things and I did not felt like writing a changelog for them etc).
I also have more things in mind to fix the debugging plugin, and it is just easier to not have conflicts etc (and to iterate faster then in cyccles of months ;)).

Oh I see: #5244

Well it is a shame the discussion just lost traction there.

Yeah, #5244 was my main motivation but the fix there was different.

Note that this issue is not fixed, but OK for me to have it closed.

[nicoddemus]

I also have more things in mind to fix the debugging plugin, and it is just easier to not have conflicts etc (and to iterate faster then in cyccles of months ;)).

If you have a problem with our iteration speed and/or believe our PRs take too long to be merged, we should discuss/address this. IMHO this is the responsible thing for a core maintainer to do, not just fix bugs in your fork and leave the core as is.

This issue is particularly concerning, because it seems you addressed my concerns in the initial PR (about leaking state after pytest.main()), but forego trying to get this upstream and just applied to your fork instead.

To be clear: I don't have anything against someone fixing things in their fork and having no interest in pushing this upstream (after all it's all part of the license and everyone is welcome to it, after all it is OSS), I just don't think it is right for a core mainteiner to assume this posture.

Don't you agree?

[blueyed]

@nicoddemus
Given this particular issue / the referenced fix: I've never said that I would not merge it back, have I?
After all putting a reference here helps to do so when the time would come. (I cannot see why there would be a rush suddenly, when you've not followed up since May before already, and given no interest from others)
But apart from that it is still a fix currently that you did not want back then already, just that it is now wrapped in instances, instead of stashed on the class. Therefore it "might still leak" as you've put it, and would not be fitting here still AFAICS.
That's why I've mentioned that I have more plans in this regard, and am happy to keep it on my fork for now.

Edit: it was also you that closed the issue, I've explicitly re-opened it giving previous feedback about closing thing which I consider to be fixed for myself.

As for the rest I consider this to be disrespectful and disheartening, but also off-topic here.
I've written some more, so feel free to follow up via PM.

(And just to be clear: I do not really have an alternative to having a fork in the first place, given that there are things you do not like to have here in general (dict for parametrize, capman-suspend-on-reading-from-stdin, removing hypothesis etc, and just being able to fix/improve minor issues on the go - a lot of them being sent back here also, which you should know / be aware of!))

[nicoddemus]

@blueyed first of all, let me apologize, I understand that you are upset, and I am sorry.

About this issue, the impression that I had initially was that you have fixed the problem in your fork and had no intention to bring the fix to the main repository. There's no rush to apply the fix, but any fix is welcome. After all you did say:

I also have more things in mind to fix the debugging plugin, and it is just easier to not have conflicts etc (and to iterate faster then in cyccles of months ;)).

This is mainly what gave me that impression, but if that was not the case I'm sorry.

About your fork with rejected features I think that's great (btw, it was not "me" that rejected parametrize with dicts, other maintainers felt the same way), I was concerned about bug fixes only.

Also, I know it can be frustrating to have PRs sitting there for weeks or months, but sometimes people are busy or things slip through the cracks, feel free to ping them instead of closing them.

Finally, sorry again for my response, it was not my intention to leave you disheartened.

Cheers