Skip to content

[WebServerBundle] Store pid file in project directory instead #29160

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.

Sign up for GitHub

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

Jump to bottom
Closed
nclavaud opened this issue Nov 9, 2018 · 8 comments
Closed

[WebServerBundle] Store pid file in project directory instead #29160

nclavaud opened this issue Nov 9, 2018 · 8 comments
Labels
Feature WebServerBundle

Comments

@nclavaud
Copy link

nclavaud commented Nov 9, 2018 •
edited
Loading

Description

Commit 6f689d6 introduced a pid file to keep track of the servers that have been started using command server:start. As stated in the pull request description:

the web server now stores its address in a pid file stored in the current directory

The current directory is the directory where the shell command was launched, and not necessarily the project directory. This can lead to wrong reports if the same command is run twice from different directories, or if different servers (from different apps) are started from the same directory.

For these reasons, I suggest using the project directory instead.

Example (same app, change current directory)

Before

php bin/console server:start  // [OK] Server listening on http://127.0.0.1:8000

cd bin
php console server:start      // [OK] Server listening on http://127.0.0.1:8001

After

php bin/console server:start  // [OK] Server listening on http://127.0.0.1:8000

cd bin
php console server:start      // [ERROR] The web server is already running (listening on http://127.0.0.1:8000).

Another example (different apps, same directory)

Before

php /var/www/app1/bin/console server:start  // [OK] Server listening on http://127.0.0.1:8000
php /var/www/app2/bin/console server:start  // [ERROR] The web server is already running (listening on http://127.0.0.1:8000).

After

php /var/www/app1/bin/console server:start  // [OK] Server listening on http://127.0.0.1:8000
php /var/www/app2/bin/console server:start  // [OK] Server listening on http://127.0.0.1:8001

What do you think?
@ro0NL
Copy link
Contributor

ro0NL commented Nov 10, 2018 •
edited
Loading

note you can specify the --pidfile option. Not sure injecting the project dir etc. is worth it; the current default is also sensible IMHO.

See https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/WebServerBundle/Command/ServerStartCommand.php#L59

@nclavaud
Copy link
Author

note you can specify the --pidfile option

I know, but still, I think the default makes little sense. Bad DX to me.

@nicolas-grekas
Copy link
Member

can't we store it in the cache dir?

@nclavaud
Copy link
Author

can't we store it in the cache dir?

I'm not sure we want the pid file to be deleted when running cache:clear, do we? The server will keep running after the cache has been wiped, but we would loose track of it.

This pid file holds server state, and in the linux philosophy this would belong to directory /run, I guess, but there is no such run/ or var/run/ directory in Symfony's default directory structure.

Any objection to use the project directory as the default pid file location?

@nicolas-grekas
Copy link
Member

The cache dir is not deleted on cache:clear
My suggestion is to store the pid in var/cache/something.pid - not in var/cache/dev/something.pid

@nclavaud
Copy link
Author

Would be fine, then!
Will open a PR. :-)

@gido
Copy link
Contributor

gido commented Dec 8, 2018

I picked this issue when working during the #SymfonyConHackday2018 in Lisbon and I'm not sure if using the the root cache directory (var/cache and no var/cache/dev is ideal because you need to make a lot of assumption on how the cache directory structured.

The naive implementation could be just: dirname($cacheDir) but you can put what you want into Kernel::getCacheDir(). This could lead to more issues that is solve.

Using the project directory (%kernel.project_dir%) to solve this looks just fine for me but maybe I miss something?

@Simperfit
Copy link
Contributor

@nclavaud or @gido are you still working on this, do you want some help, if so could you please open a PR to see what needs to be done or if everything is OK.
Thanks :).

@jschaedl jschaedl mentioned this issue Apr 26, 2019
Merged
@fabpot fabpot closed this as completed Apr 29, 2019
fabpot added a commit that referenced this issue Apr 29, 2019
@fabpot
feature #31280 [WebServerBundle] Change the default pidfile location …
707b1df 
…to cache directory (jschaedl)

This PR was squashed before being merged into the 4.3-dev branch (closes #31280).

Discussion
----------

[WebServerBundle] Change the default pidfile location to cache directory

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #29160   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | tbd.

<!--
Write a short README entry for your feature/bugfix here (replace this comment block.)
This will help people understand your PR and can be used as a start of the Doc PR.
Additionally:
 - Bug fixes must be submitted against the lowest branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too).
 - Features and deprecations must be submitted against the master branch.
-->

Commits
-------

2e14b6e [WebServerBundle] Change the default pidfile location to cache directory
@simmonspaul simmonspaul mentioned this issue Mar 10, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Feature WebServerBundle
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@fabpot @gido @nicolas-grekas @ro0NL @nclavaud @xabbuh @Simperfit