Ladybird+SQLServer: Migrate server launch to Core::IPCProcess::spawn #24398
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
👀 pr-needs-review
Both SQLServer and RequestServer are singletons; however, launch_sql_server_process() was still using an older spawn strategy that performs manual forking instead of the new higher level process spawn abstraction in LibCore. This opened up a few areas for simplification: No longer passing the PID file to SQLServer, and no longer using MultiServer explicitly to manually handle new client connections, as the AK::IDAllocator helper can auto create new client IDs
|
LibWeb CI for this Pull Request seems to stall on LibWeb tests. Running them locally seems to work fine, I've resubmitted a few times now, so not sure what else to try 🤔 |
|
RequestServer isn't really a singleton process, we just happen to only launch one. But if another app wants to use RequestServer, it will be able to launch its own, rather than re-use Ladybird's instance. SQLServer does use a singleton process approach on purpose. If we launch Ladybird and the sql REPL, then we do want them talking to the same SQLServer process. |
|
Ah I see, my conversation here lead me to believe they both were singletons and therefore should share the same launching mechanism. Thanks for the clarification! |
github-actions
bot
removed
the
👀 pr-needs-review
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Just saw an inconsistency in how SQLServer and RequestServer were being spawned, and since they are both singleton processes they should be able to be launched the same way. The newer strategy also allowed for some cleanup in how SQLServer handles client connections.
sqlserver-localstorage.mp4