getnikola / nikola

A static website and blog generator
https://getnikola.com/
MIT License
2.58k stars 443 forks source link

Move asyncio loop management from plugin to main. #3729

Closed aknrdureegaesr closed 5 months ago

aknrdureegaesr commented 6 months ago

Pull Request Checklist

Description

The code review remark in my recent PR made me think: Where does loop management really belong?

I think it should not be done by a mere plugin, such as auto.

Doing so was ok as long as auto was the only place where the loop was used. The moment something else used the loop, problems started. This was the case when I wrote my tests. It could also have been the case if some other plugin different from auto also decides to use the loop, although many such uses might go through without issues. Still, auto's implicit insistence "the loop is mine" makes it a somewhat unpleasant neighbor.

With this pull request, I make a suggestion how else to organize this:

Kwpolska commented 6 months ago

I tested the code with Python 3.8 and 3.11 on Windows (3.12 would require me to install Visual C++ first…). It works fine; 3.8 throws an exception if ^Cing during a rebuild, but it’s a minor thing. But then I removed all changes from __main__.py and things were okay as well, and the 3.8 exception was gone. I would be open to dropping Python 3.7 to avoid having any special asyncio config.

The exception 3.8 threw (I got it to happen twice):

[2024-01-06 20:36:36] INFO: auto: Server is shutting down.
Exception ignored in: <function BaseSubprocessTransport.__del__ at 0x000001B2182C09D0>
Traceback (most recent call last):
  File "C:\Program Files\Python38\lib\asyncio\base_subprocess.py", line 126, in __del__
    self.close()
  File "C:\Program Files\Python38\lib\asyncio\base_subprocess.py", line 104, in close
    proto.pipe.close()
  File "C:\Program Files\Python38\lib\asyncio\proactor_events.py", line 108, in close
    self._loop.call_soon(self._call_connection_lost, None)
  File "C:\Program Files\Python38\lib\asyncio\base_events.py", line 719, in call_soon
    self._check_closed()
  File "C:\Program Files\Python38\lib\asyncio\base_events.py", line 508, in _check_closed
    raise RuntimeError('Event loop is closed')
RuntimeError: Event loop is closed
Exception ignored in: <function _ProactorBasePipeTransport.__del__ at 0x000001B2182F00D0>
Traceback (most recent call last):
  File "C:\Program Files\Python38\lib\asyncio\proactor_events.py", line 115, in __del__
    _warn(f"unclosed transport {self!r}", ResourceWarning, source=self)
  File "C:\Program Files\Python38\lib\asyncio\proactor_events.py", line 79, in __repr__
    info.append(f'fd={self._sock.fileno()}')
  File "C:\Program Files\Python38\lib\asyncio\windows_utils.py", line 102, in fileno
    raise ValueError("I/O operation on closed pipe")
ValueError: I/O operation on closed pipe
aknrdureegaesr commented 6 months ago

Decades ago, with my private machine, I went directly from DOS to Linux. I had to use Windows some professionally, but on computers privately used by me, I never had Windows. So I have no Windows machine at my disposal, short of borrowing one. In summary, I feel I'm a less-than-perfect person to debug any Windows-specific exceptions.

That said, what is a good way to proceed now? I can offer to remove the special code from main and the support announcement for 3.7 from setup.py, so the 3.7 drop is in the commit that made it necessary, plus an announcement about that drop in CHANGES.txt.

Would that be helpful?

Kwpolska commented 6 months ago

Yeah, please remove the __main__.py code and mentions of 3.7 from setup.py and the GitHub Actions definitions.

aknrdureegaesr commented 6 months ago

There is something weird going on with the tests here. I removed the Nikola tests (Python 3.7 on ubuntu-latest) from .github/workflows/ci.yml, but there is still the message "Some checks haven't completed yet" waiting for that test.

Not entirely sure how Github works this detail. Maybe some test expectancy from the master (sic!) branch rather than from the branch that wants the merge are used? This could make sense from a security point of view.

Kwpolska commented 6 months ago

There is something weird going on with the tests here. I removed the Nikola tests (Python 3.7 on ubuntu-latest) from .github/workflows/ci.yml, but there is still the message "Some checks haven't completed yet" waiting for that test.

Not entirely sure how Github works this detail. Maybe some test expectancy from the master (sic!) branch rather than from the branch that wants the merge are used? This could make sense from a security point of view.

The list of expected checks is defined in repository settings. I removed 3.7 from that list.

aknrdureegaesr commented 6 months ago

My present plan is to wait for #3740 (which duplicates some of the changes here), and only afterwards clean up this branch (remove the duplicate material and rebase).

Update 2024-01-24: Done. This wants to be merged again.

aknrdureegaesr commented 5 months ago

It is maybe better to mention this in a comment by itself: This PR has been rebased to fit on the current master as of 2f5cf49a8da2 from last weekend, and can be merged now, as far as I can see.