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

WIP: Fix #790: preview server - Uses Thread instead of Process for Linux #837

Merged
merged 13 commits into from
Sep 23, 2024

Conversation

Nitorac
Copy link

@Nitorac Nitorac commented Jun 16, 2024

Workaround to fix Preview of rst files not working #790.

To limit any retrocompatibility issues, a Thread is used (instead of the current Process) only if we are on Linux.

@Nitorac Nitorac force-pushed the fix-multiprocess-python312 branch from 677a3cd to 1453956 Compare June 16, 2024 17:43
@Nitorac
Copy link
Author

Nitorac commented Jun 16, 2024

(sorry for the force push, the commit had not the correct author)

@Nitorac Nitorac changed the title Fix #790: preview server - Uses Thread instead of Process for Python 3.12+ WIP: Fix #790: preview server - Uses Thread instead of Process for Python 3.12+ Jun 16, 2024
@Nitorac Nitorac marked this pull request as draft June 16, 2024 18:56
@Nitorac Nitorac changed the title WIP: Fix #790: preview server - Uses Thread instead of Process for Python 3.12+ WIP: Fix #790: preview server - Uses Thread instead of Process for Linux Sep 15, 2024
@Nitorac Nitorac marked this pull request as ready for review September 15, 2024 03:46
@Nitorac
Copy link
Author

Nitorac commented Sep 15, 2024

Tested successfully on Ubuntu 24.04 with Python 3.12.3 (for the Thread part) and on Windows 11 with Python 3.12.6 (for the old Process part)

I'm open to any feedback

@alcarney
Copy link
Member

Awesome, thanks for this!

I've only taken a quick glance, but it looks good to me so far. I'll try and have a proper look over the next week or so :)

@alcarney
Copy link
Member

Ok, I've resurrected the ci pipelines for the 0.x branch and have (hopefully! 🤞) reconfigured things so that pushing a release from there works also.

However, having re-read #790 I noticed that the issue was originally reported on MacOS - could you tweak this PR so that a Thread is used for all platforms? (Apologies for not noticing sooner! 😅)

While you're at it could you also add a changelog entry in a file lib/esbonio/changes/790.fix.rst (See here for an example). Feel free to append by @Nitorac to your message so that you get the credit for the contribution on the GitHub release page :)

@Nitorac
Copy link
Author

Nitorac commented Sep 23, 2024

Ok, I've resurrected the ci pipelines for the 0.x branch and have (hopefully! 🤞) reconfigured things so that pushing a release from there works also.

Good news !

However, having re-read #790 I noticed that the issue was originally reported on MacOS - could you tweak this PR so that a Thread is used for all platforms? (Apologies for not noticing sooner! 😅)

Oh yeah you're right, I didn't pay attention to that, it's hard to understand what environments are affected !
So I agree with you, I will edit the PR to remove the Process part entirely.

While you're at it could you also add a changelog entry in a file lib/esbonio/changes/790.fix.rst (See here for an example). Feel free to append by @Nitorac to your message so that you get the credit for the contribution on the GitHub release page :)

I will add it. Thanks for your explanations !

@Nitorac
Copy link
Author

Nitorac commented Sep 23, 2024

@alcarney The main part is done, but I have a question :
I couldn't understand why on non-Linux systems, a Queue was used to fetch the preview server port after creating it. (

)

Since I use a Thread now, I have just removed it, but I don't want to make any mistakes here !

Btw, as before, I tested and it worked on Ubuntu 24.04 with Python 3.12.3 and on Windows 11 with Python 3.12.6.

@alcarney
Copy link
Member

I couldn't understand why on non-Linux systems, a Queue was used to fetch the preview server port after creating it

I don't remember the exact details, but in order to share objects with a multiprocessing.Process they must be pickle-able. For some reason the server object was only pickle-able on Linux, so the Queue was used on other platforms as a work around.

Now that we're using a Thread that's no longer necessary :)

Copy link
Member

@alcarney alcarney left a comment

Choose a reason for hiding this comment

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

Thanks again for working on this - looks good to me! :)

@alcarney alcarney merged commit 64c9097 into swyddfa:0.x Sep 23, 2024
8 of 10 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