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 video-streamer zombie processes #1590

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

walesch-yan
Copy link
Collaborator

This PR closes #1577 .

This PR updates pyproject.toml to use the version of mxcubecore fixing the above issue. The fix was introduced in mxcube/mxcubecore#1169 . It also moves the video-streamer dependency into mxcubecore and removes it from here.

Additionally, I had to modify the tests, previously in our atexit register, we could decide to not kill any processes, as it was called in mxcubeweb. This is now different and we will see some ProcessLookUpError since they are not well synchronized with pytests handlers. Instead, this PR introduces a monkeypatch for the atexit.register function and to make sure to kill any child processes psutil is used after each test.

Bonus of this PR:
Using the latest version of mxcubecore will also fix a security vulnerability introduced by jinja2.

@walesch-yan walesch-yan force-pushed the yw-fix-video-streamer-closure branch 2 times, most recently from 113ea91 to 093273c Compare March 13, 2025 14:33
@walesch-yan walesch-yan force-pushed the yw-fix-video-streamer-closure branch from 093273c to 0726ec0 Compare March 13, 2025 14:47
@walesch-yan
Copy link
Collaborator Author

So In this line, when running manually the pre-commits on python 3.10 it changed

python_version == "3.10"

to

python_version >= "3.10" and python_version < "3.11"

which was not detected by the automatic local pre-commits (3.10) but crashed the CI running on 3.11, just in case somebody encounters this issue

Comment on lines -17 to -18
# This is to avoid `Server.kill_processes`, that makes the build return non-zero
server.flask.testing = True
Copy link
Collaborator Author

@walesch-yan walesch-yan Mar 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm removing this testing flag on a few occasions in the code, since it is now no longer needed

@fabcor-maxiv
Copy link
Contributor

So In this line, when running manually the pre-commits on python 3.10 it changed

python_version == "3.10"

to

python_version >= "3.10" and python_version < "3.11"

which was not detected by the automatic local pre-commits (3.10) but crashed the CI running on 3.11, just in case somebody encounters this issue

That is curious...

Could it be that somewhere somehow a different version of Poetry was used? I think the version is pinned in the pre-commit hook, so it should be safe from such issues.

@walesch-yan
Copy link
Collaborator Author

That is curious...

Could it be that somewhere somehow a different version of Poetry was used? I think the version is pinned in the pre-commit hook, so it should be safe from such issues.

I thought so as well at the beginning and my first try was to use the 1.8.0 version (we have 1.9.0), but It would give me the same faulty result, it was only when I changed the python version that this was resolved

@fabcor-maxiv
Copy link
Contributor

That is curious...
Could it be that somewhere somehow a different version of Poetry was used? I think the version is pinned in the pre-commit hook, so it should be safe from such issues.

I thought so as well at the beginning and my first try was to use the 1.8.0 version (we have 1.9.0), but It would give me the same faulty result, it was only when I changed the python version that this was resolved

OK... that does not make much sense to me. Some bug somewhere maybe...

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.

Stopping MXCuBE-Web leaves video-streamer running
2 participants