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

Move asyncio loop management from plugin to main. #3729

Merged

Conversation

aknrdureegaesr
Copy link
Contributor

@aknrdureegaesr aknrdureegaesr commented Jan 4, 2024

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:

  • Loop handling is removed from the auto plugin.
  • It is moved to main.
  • It is re-organized, from some startup code at auto module load time and some cleanup code in the middle of auto, to a Python-ish context handler.
  • The test of auto could be cleaned up and looks better now. In particular, the doubtful remark that triggered the code review comment vanished.

@aknrdureegaesr aknrdureegaesr force-pushed the event_loop_not_owned_by_plugin branch 4 times, most recently from 3f32cf4 to 6072dcb Compare January 4, 2024 20:46
@aknrdureegaesr aknrdureegaesr marked this pull request as draft January 4, 2024 21:04
@aknrdureegaesr aknrdureegaesr changed the title Move asyncio loop management from plugin to main. Draft: Move asyncio loop management from plugin to main. Jan 4, 2024
@aknrdureegaesr aknrdureegaesr marked this pull request as ready for review January 4, 2024 21:40
@aknrdureegaesr aknrdureegaesr changed the title Draft: Move asyncio loop management from plugin to main. Move asyncio loop management from plugin to main. Jan 4, 2024
@Kwpolska
Copy link
Member

Kwpolska commented Jan 6, 2024

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
Copy link
Contributor Author

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
Copy link
Member

Kwpolska commented Jan 9, 2024

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

@aknrdureegaesr aknrdureegaesr force-pushed the event_loop_not_owned_by_plugin branch 2 times, most recently from 969b9a2 to ec370bc Compare January 11, 2024 09:08
@aknrdureegaesr
Copy link
Contributor Author

aknrdureegaesr commented Jan 11, 2024

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
Copy link
Member

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
Copy link
Contributor Author

aknrdureegaesr commented Jan 17, 2024

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 aknrdureegaesr force-pushed the event_loop_not_owned_by_plugin branch 2 times, most recently from f51fe1b to 5e820ae Compare January 24, 2024 10:35
@aknrdureegaesr
Copy link
Contributor Author

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 2f5cf49 from last weekend, and can be merged now, as far as I can see.

@Kwpolska Kwpolska merged commit c0362ef into getnikola:master Jan 24, 2024
11 checks passed
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.

2 participants