Fixes signal that only works in main thread of the main interpreter

[AthKouloumvakos]

Signal only works in main thread of the main interpreter so this commit

• Adds a check in the _add_shutdown_signals where if the download process is out of the main thread of the main interpreter, it gives a warning and returns early without adding a shutdown signal.
• Adds a test where a file is successfully downloaded within a custom thread.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3b049c5) 90.07% compared to head (8f38c21) 90.31%.

❗ Current head 8f38c21 differs from pull request most recent head 51f64f1. Consider uploading reports for the commit 51f64f1 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #133      +/-   ##
==========================================
+ Coverage   90.07%   90.31%   +0.23%     
==========================================
  Files           5        5              
  Lines         635      640       +5     
==========================================
+ Hits          572      578       +6     
+ Misses         63       62       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Cadair]

Sorry it's taken me an insane amount of time to actually look at this.

Looking at the warning output of the the CI builds: https://github.com/Cadair/parfive/actions/runs/6156492456/job/16705281425?pr=133#step:10:360 We should test that the warning is raised using pytest.warns in your test.

Also there's some annoying error where shutting down the event loop in a thread raises an error, but I am not sure if that's our error or not.

[AthKouloumvakos]

Hi again and thank you for looking at this!

As you suggested I will add a check on the test that the warning is raised. I will use pytest.warns so I do my reading now and I will push this soon.

I have also explored the warnings of the tests further. These are ~related to the same signal issue but there are not related to the new additions.

I started from a previous version to make some tests (this commit ae5f6bc, just before my version).

The warning I get from pytest is

  /Users/kouloa1/opt/anaconda3/envs/parfive/lib/python3.9/site-packages/_pytest/unraisableexception.py:78: PytestUnraisableExceptionWarning: Exception ignored in: <function BaseEventLoop.__del__ at 0x10c9f85e0>
  
  Traceback (most recent call last):
    File "/Users/user/opt/anaconda3/envs/parfive/lib/python3.9/asyncio/base_events.py", line 688, in __del__
      self.close()
    File "/Users/user/opt/anaconda3/envs/parfive/lib/python3.9/asyncio/unix_events.py", line 61, in close
      self.remove_signal_handler(sig)
    File "/Users/user/opt/anaconda3/envs/parfive/lib/python3.9/asyncio/unix_events.py", line 150, in remove_signal_handler
      signal.signal(sig, handler)
    File "/Users/user/opt/anaconda3/envs/parfive/lib/python3.9/signal.py", line 56, in signal
      handler = _signal.signal(_enum_to_int(signalnum), _enum_to_int(handler))
  ValueError: signal only works in main thread of the main interpreter
  
    warnings.warn(pytest.PytestUnraisableExceptionWarning(msg))

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html

The ValueError: signal only works in main thread of the main interpreter affects the test_async_download and test_download_unique. I am not sure why these two run out of the main interpreter but the issue is essentially the same that we deal here. I think this is a signal issue but I haven't found a definite solution.

[Cadair]

😭 Why does Python 3.8 hate us lol

I think this is pretty much good to go if we can make the py3.8 build happy.

[AthKouloumvakos]

I do not think this fail is py38 dependent. The CI test passed on py38 ubuntu but failed on windows, so I think it is os dependent.

According to the error,

E           Failed: DID NOT WARN. No warnings of type (<class 'UserWarning'>,) were emitted.
E           The list of emitted warnings is: [].

no warning is emitted when runing the test on windows-latest, as if the condition if threading.current_thread() != threading.main_thread(): at _add_shutdown_signals never passed. I think that, even if the test run is on the CustomThread this is considered as the main_thread() and threading.current_thread() == threading.main_thread() always. I wonder if this propably means that signal error "Signal only works in main thread of the main interpreter" is not a problem for windows and never rise an error, but I cannot test this because I do not use windows. If this is the case then we can ingnore this for os windows and adjust also the test_download_out_of_main_thread to skip in this case.

Any suggestions?

[AthKouloumvakos]

Hi @Cadair I think now I understand better what is happening with the issue.

As I previously mentioned the test_download_out_of_main_thread fails only on Windows and now that I understand this better. I think this is "normal" and I will explain. The UserWarning is never emitted because initially the _add_shutdown_signals is skipped for windows so this is why the test fails. So, one fix is to skip the test for Windows, similar to other tests with similar issues. I have made a commit on this so you can check it. Another solution which might be more preferable is to emit the warning in both cases, e.g.

    def _add_shutdown_signals(loop, task):
        if os.name == "nt" or threading.current_thread() != threading.main_thread():
            warnings.warn(
                "This download has been started in Windows or a thread which is not the main thread. You will not be able to interrupt the download.",
                UserWarning,
            )
            return

        for sig in (signal.SIGINT, signal.SIGTERM):
            loop.add_signal_handler(sig, task.cancel)

[Cadair]

hmm, it seems that the test this PR added is flakey on macos, i.e. sometimes it passes and sometimes it fails.