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

Fix stopping service on windows (#66) #175

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

jv3ga
Copy link

@jv3ga jv3ga commented Apr 15, 2024

This fixes the #66 issue, it's happening because on windows the ctrl+c signal propagates over all subprocess (like pressing ctrl+c on each subprocess), then the sentinel will see that the workers and all other subprocess crashed, so it will restart each one. With this fix, we tell to workers, pushers and monitor to ignore that signal. And the cluster control the first signal stoping all subrpoces gracefully

@coveralls
Copy link
Collaborator

coveralls commented Apr 15, 2024

Pull Request Test Coverage Report for Build 9012968557

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 14 of 25 (56.0%) changed or added relevant lines in 4 files are covered.
  • 6 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-0.4%) to 89.513%

Changes Missing Coverage Covered Lines Changed/Added Lines %
django_q/monitor.py 3 5 60.0%
django_q/pusher.py 3 5 60.0%
django_q/worker.py 3 5 60.0%
django_q/cluster.py 5 10 50.0%
Files with Coverage Reduction New Missed Lines %
django_q/tests/test_scheduler.py 1 99.52%
django_q/utils.py 1 89.13%
django_q/monitor.py 1 91.96%
django_q/core_signing.py 3 81.63%
Totals Coverage Status
Change from base Build 8159074719: -0.4%
Covered Lines: 3346
Relevant Lines: 3738

💛 - Coveralls

@GDay
Copy link
Collaborator

GDay commented Apr 16, 2024

Thanks! This looks good. I will test it asap on a windows machine and the merge.

@HairlessVillager
Copy link

Thanks for your contribution🥰 I've tested on my Windows 10 machine and it can be stoped by Ctrl-C, except a ModuleNotFoundError: No module named 'win32api'. Looks like it can be fixed quickly.

I pulled jv3ga/django-q2 and installed it via pip install -e .. My pip list:

Package            Version     Editable project location
------------------ ----------- ----------------------------------
aiohttp            3.8.6
alembic            1.13.1
annotated-types    0.6.0
anyio              3.7.1
apprise            1.7.1
APScheduler        3.10.4
asgi-lifespan      2.1.0
asgiref            3.8.1
async-timeout      4.0.3
asyncpg            0.29.0
cachetools         5.3.2
croniter           2.0.1
dateparser         1.2.0
Django             5.0
django-picklefield 3.2
django-q2          1.6.2       E:\code\python\django-q2\django-q2
dnspython          2.5.0
docker             6.1.3
email-validator    2.1.0.post1
google-auth        2.27.0
graphviz           0.20.1
greenlet           3.0.2
griffe             0.39.1
h11                0.14.0
httpcore           0.17.3
httpx              0.24.1
kubernetes         29.0.0
lxml               4.9.4
Mako               1.3.1
oauthlib           3.2.2
orjson             3.9.12
pendulum           2.1.2
Pillow             10.0.1
pip                23.3.1
pycryptodomex      3.19.1
pygame             2.5.1
pynvim             0.5.0
pytzdata           2020.1
qrcode             7.4.2
qrcode-terminal    0.8
readchar           4.0.5
requests-oauthlib  1.3.1
rich               13.7.0
rsa                4.9
setuptools         68.2.2
sniffio            1.3.0
sqlparse           0.5.0
tqdm               4.66.1
typer              0.9.0
tzdata             2024.1
tzlocal            5.2
ujson              5.9.0
uvicorn            0.27.0
wheel              0.43.0

Then I ran:

  1. django-admin startproject mq_required
  2. cd mq_required
  3. python manage.py migrate
  4. python manage.py qcluster

and a ModuleNotFoundError occured for win32api was not installed. The traceback:

(django-q2) E:\code\python\django-q2\mq_required>python manage.py qcluster
Traceback (most recent call last):
  File "E:\code\python\django-q2\mq_required\manage.py", line 22, in <module>
    main()
  File "E:\code\python\django-q2\mq_required\manage.py", line 18, in main
    execute_from_command_line(sys.argv)
  File "D:\Anaconda\anaconda3\envs\django-q2\Lib\site-packages\django\core\management\__init__.py", line 442, in execute_from_command_line
    utility.execute()
  File "D:\Anaconda\anaconda3\envs\django-q2\Lib\site-packages\django\core\management\__init__.py", line 436, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "D:\Anaconda\anaconda3\envs\django-q2\Lib\site-packages\django\core\management\base.py", line 412, in run_from_argv
    self.execute(*args, **cmd_options)
  File "D:\Anaconda\anaconda3\envs\django-q2\Lib\site-packages\django\core\management\base.py", line 458, in execute
    output = self.handle(*args, **options)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "E:\code\python\django-q2\django-q2\django_q\management\commands\qcluster.py", line 36, in handle
    q = Cluster()
        ^^^^^^^^^
  File "E:\code\python\django-q2\django-q2\django_q\cluster.py", line 48, in __init__
    import win32api
ModuleNotFoundError: No module named 'win32api'

After I installed it, the qcluster started up, and can be stoped by Ctrl-C.

@jv3ga
Copy link
Author

jv3ga commented May 8, 2024

Hi,

Thanks for the tip. I added this on the file requirements.txt:

pywin32==306; platform_system == "Windows" and python_version >= "3.7" \
    --hash=sha256:39b61c15272833b5c329a2989999dcae836b1eed650252ab1b7bfbe1d59f30f4

But I dont really underesteand why some requirements has two hashes, for example:

ansicon==1.89.0; platform_system == "Windows" and python_version >= "2.7" \
    --hash=sha256:f1def52d17f65c2c9682cf8370c03f541f410c1752d6a14029f97318e4b9dfec \
    --hash=sha256:e4d039def5768a47e4afec8e89e83ec3ae5a26bf00ad851f914d1240b444d2b1

Can someone explainme how to get the second hash?
Thanks!!

@GDay
Copy link
Collaborator

GDay commented May 8, 2024

@jv3ga Could you add it to the project.toml file instead (with a Windows condition, similar to this: https://python-poetry.org/docs/dependency-specification/#using-environment-markers)? And then poetry can render the requirements.txt file, so you don't have to worry about that.
The build system uses the project.toml file.

Could you also separate the oracle fix and put that in a new PR, please?

@jv3ga
Copy link
Author

jv3ga commented May 9, 2024

Thanks for your answer, I'm kind of newie collaborating in open source projects.
Hope everything is OK. New pull request: #186

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.

None yet

4 participants