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

All Crystal frameworks are using a now deprecated function #6443

Open
jwoertink opened this issue Jun 15, 2023 · 6 comments
Open

All Crystal frameworks are using a now deprecated function #6443

jwoertink opened this issue Jun 15, 2023 · 6 comments

Comments

@jwoertink
Copy link
Contributor

As of Crystal 1.8, Process.fork is now deprecated https://github.com/crystal-lang/crystal/blob/master/CHANGELOG.md#180-2023-04-14

In src/lucky.cr:13:3

 18 | Process.fork do
              ^---
Warning: Deprecated Process.fork. Fork is no longer supported.

A total of 1 warnings were found.

Here's the PR where it was deprecated.

All of the current Crystal frameworks use these lines in order to spin up multiple processes for speed.

System.cpu_count.times do |i|
Process.fork do

System.cpu_count.times do |i|
Process.fork do

Since Crystal 1.8 deprecates this, and Crystal 1.9 could potentially remove it, we may need to find a new way to test all of these.

@waghanza
Copy link
Collaborator

Thanks for catch up @jeremyevans, we definitely need to upgrade codebase.

Could also be great to have some warning on deprecation (I mean using CI or else)

@waghanza
Copy link
Collaborator

@crimson-knight
Copy link
Contributor

@jwoertink @waghanza I made #6558 to test using this approach. I currently use this for a small binary that needs to trigger multiple child processes.

Unfortunately I couldn't get the test suite to run on my local machine because docker-machine requires VirtualBox which would only install if I wasn't on an M1. I can try playing around with this again later if no one else gets it working.

@Blacksmoke16
Copy link
Contributor

Could just ignore it, it's only deprecated so not like its going anywhere anytime soon. Maybe by the time Crystal 2.0 comes around, MT support will be better supported and can just leverage that. Might even work already, but haven't did much with it yet. Plus multi process doesn't have the same overhead so iirc gives better perf anyway.

@stakach
Copy link
Contributor

stakach commented Jul 31, 2023

Spider-gazelle already supports non-forked clustering, replacing this line with:

server.cluster(process_count, "-w", "--workers") if process_count != 1

(and threads if built with threading enabled - although then you'd want to disable clustering)

@crimson-knight
Copy link
Contributor

@jwoertink I switched the Amber benchmark to use Process.new and that appears to be working just fine:

https://github.com/the-benchmarker/web-frameworks/blob/master/crystal/amber/src/amber.cr

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

No branches or pull requests

5 participants