Since this isn't currently tested, should we add a test for this?

I suppose so. I guess one strategy for testing would be to run a simple script as a subprocess with pg.exit() at the end and make sure it returns 0. Does that sound reasonable?

I suppose so. I guess one strategy for testing would be to run a simple script as a subprocess with pg.exit() at the end and make sure it returns 0. Does that sound reasonable?

Considering the initial failure mode could be triggered by

import pyqtgraph as pg
pg.exit()

Firing off a subprocess and exiting at the end of it might be a bit overkill, but as long as it's not too much effort I'm a 👍

Just calling pg.exit() in the test function appears to kill the test process unless you're using pytest-xdist.

I added a test using a subprocess like I described. I checked that it does fail if you add something like os.close(1000000) to exit() (though that shows up as a TimeoutExpired exception) or if you change the last line to os._exit(-1). So I don't think the new test should cause any issues but I'm not sure it catches everything in a nice way.

Welp it didn't pass on MacOS. Maybe the timeout is too small? Maybe the timeout isn't actually necessary, but when I tested with os.close(1000000) to cause the Bad file descriptor error, the process hangs.

Just calling pg.exit() in the test function appears to kill the test process unless you're using pytest-xdist.

I added a test using a subprocess like I described. I checked that it does fail if you add something like os.close(1000000) to exit() (though that shows up as a TimeoutExpired exception) or if you change the last line to os._exit(-1). So I don't think the new test should cause any issues but I'm not sure it catches everything in a nice way.

Looks like the Python2 CI pipelines aren't happy:

pyqtgraph/tests/test_exit_crash.py::test_pg_exit 
[gw0] FAILED pyqtgraph/tests/test_exit_crash.py::test_pg_exit 

________________________________ test_pg_exit _________________________________
[gw0] win32 -- Python 2.7.16 C:\Miniconda\envs\test-environment-2.7\python.exe

    def test_pg_exit():
        # test the pg.exit() function
        code = textwrap.dedent("""
            import pyqtgraph as pg
            app = pg.mkQApp()
            pg.plot()
            pg.exit()
        """)
>       ret = subprocess.call([sys.executable, '-c', code], timeout=1)

..\pyqtgraph\tests\test_exit_crash.py:57: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

popenargs = (['C:\\Miniconda\\envs\\test-environment-2.7\\python.exe', '-c', '\nimport pyqtgraph as pg\napp = pg.mkQApp()\npg.plot()\npg.exit()\n'],)
kwargs = {'timeout': 1}

    def call(*popenargs, **kwargs):
        """Run command with arguments.  Wait for command to complete, then
        return the returncode attribute.
    
        The arguments are the same as for the Popen constructor.  Example:
    
        retcode = call(["ls", "-l"])
        """
>       return Popen(*popenargs, **kwargs).wait()
E       TypeError: __init__() got an unexpected keyword argument 'timeout'

C:\Miniconda\envs\test-environment-2.7\lib\subprocess.py:172: TypeError

Looks like the trick was having no-timeout. Are there consequences to not having one specified?

I mentioned in #956 (comment) that not having a timeout means that if the subprocess hangs for some reason it holds everything up

I mentioned in #956 (comment) that not having a timeout means that if the subprocess hangs for some reason it holds everything up

well that's less than ideal; looking through the python2 subprocess docs, doesn't look like it supports timeout being specified :/ ...that feature appears to have been added in python 3.3

Don't we have the pytest-faulthandler timeout for exactly this purpose?

Even with pytest-faulthandler, the test process never finishes without an interrupt. You can see faulthandler output gets printed after 15 seconds, but you still have to send a keyboard interrupt or something to quit. I'll rewrite the test to periodically poll the process.

Alright I think this is in pretty good shape now

Py2.7 CI is still not happy:

/usr/share/miniconda/envs/test-environment-2.7/lib/python2.7/site-packages/_pytest/python.py:507: in _importtestmodule
    mod = self.fspath.pyimport(ensuresyspath=importmode)
/usr/share/miniconda/envs/test-environment-2.7/lib/python2.7/site-packages/py/_path/local.py:701: in pyimport
    __import__(modname)
E     File "/home/vsts/work/1/s/pyqtgraph/tests/test_exit_crash.py", line 23
E       def call_with_timeout(*args, timeout=10, wait_per_poll=0.1, **kwargs):
E                                          ^
E   SyntaxError: invalid syntax

Deprecating py2 support cannot happen soon enough 😆

Doesn't help that I pretty much refuse to set up a python2 dev environment. I should use your tox config but I'm afraid it will kill my disk space 😅

Should be good to go now that I've actually waited for the full test matrix to run

LGTM, I'll merge.... I can only imagine how annoying this PR was, thanks for doing it!