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

Add ForkContext #212

Open
wants to merge 3 commits into
base: 2.x
Choose a base branch
from
Open

Add ForkContext #212

wants to merge 3 commits into from

Conversation

trowski
Copy link
Member

@trowski trowski commented Mar 1, 2025

This adds a Context implementation which uses pcntl_fork to create another process. This is a strategy which we abandoned some time ago due to the issues with copying parent context into a child, however there are particular case where that behavior can be advantageous.

This context is NOT added to DefaultContextFactory, and in fact has warnings against its use in the class docblock.

@danog Can you please check if this works for you in Psalm.

@danog
Copy link
Contributor

danog commented Mar 1, 2025

This does not work in psalm, because runContext is queueing a callback onto the event loop, which causes an immediate The event loop is already running error when hitting EventLoop::run(); unwrapping the callback fixes the issue.

@danog
Copy link
Contributor

danog commented Mar 1, 2025

Additionally, I added some extra error handling logic to handle segfaults in child processes (commonly caused by JIT usage in psalm), would love to see that in this impl as well: https://github.com/vimeo/psalm/blob/fc437245b15a7220de107bc191a3559fdbf94595/src/Psalm/Internal/Fork/ForkContext.php#L61

@danog
Copy link
Contributor

danog commented Mar 1, 2025

checkExit should probably also be executed within the constructor in a finally block as well, as vimeo/psalm#11310 is caused by a crash quite possibly before the initial IPC handshake is completed, haven't further looked into this yet...

@trowski
Copy link
Member Author

trowski commented Mar 1, 2025

This does not work in psalm, because runContext is queueing a callback onto the event loop, which causes an immediate The event loop is already running error when hitting EventLoop::run(); unwrapping the callback fixes the issue.

My local test was running from {main}, so I missed that. Unfortunately running this new context through the usual test suite crashes and burns, of course because we're forking the test process.

I removed the event loop queuing from runContext and updated ForkContext with the recent changes you made. Give it another try when you get a chance.

@kelunik
Copy link
Member

kelunik commented Mar 2, 2025

What is the use case you mentioned?

@trowski
Copy link
Member Author

trowski commented Mar 2, 2025

What is the use case you mentioned?

@danog Can you please elaborate on how you're using this in Psalm?

@danog
Copy link
Contributor

danog commented Mar 3, 2025

I'm using this in psalm to avoid serializing and sending tens of megabytes of data structures containing information about the codebase post-scanning, when starting the analysis phase.

@danog
Copy link
Contributor

danog commented Mar 7, 2025

https://github.com/danog/parallel/tree/fork-context contains some more improvements and enables (most) unit tests, working OK in Psalm!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants