Skip to content

fix: cleanup temporary directories #566

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
Merged
merged 5 commits into from
Feb 14, 2025
Merged

fix: cleanup temporary directories #566

merged 5 commits into from
Feb 14, 2025

Conversation

tucksaun
Copy link
Member

@tucksaun tucksaun commented Feb 8, 2025 •
edited
Loading

Fixes #383
Fixes #553

It also seems logical to work on cleaning any temporary directory while working on cleaning the ~/.symfony5/tmp directory so this is a slightly bigger PR.
The second commit can extracted though if we want to focus on a single directory for now.

$ find ~/.symfony5/{tmp,var,log} -type d -mindepth 1 -maxdepth 1  | wc -l
       0
$ ~/Work/src/github.com/symfony-cli/symfony-cli/symfony-cli server:start

 [WARNING] The local web server is optimized for local development and MUST never be used in a production setup.



 [WARNING] Please note that the Symfony CLI only listens on 127.0.0.1 by default since version 5.10.3.
           You can use the --allow-all-ip or --listen-ip flags to change this behavior.



 [OK] Web server listening
      The Web server is using PHP FPM 8.4.3
      https://127.0.0.1:8000


Stream the logs via symfony-cli server:log
$ find ~/.symfony5/{tmp,var,log} -type d -mindepth 1 -maxdepth 1  | wc -l
       3
$ ~/Work/src/github.com/symfony-cli/symfony-cli/symfony-cli server:stop
Stopping PHP-FPM
Stopping Web Server


 [OK] Stopped 2 process(es) successfully


$ find ~/.symfony5/{tmp,var,log} -type d -mindepth 1 -maxdepth 1  | wc -l
       0

Unfortunately cleaning up previous tmp directories does not seem like a good idea because they can still be in-use.

@tucksaun tucksaun changed the title Fix/553 fix: cleanup temporary directories Feb 8, 2025
tucksaun
tucksaun commented Feb 8, 2025
View reviewed changes
local/php/php_builtin_server.go
_ = os.MkdirAll(filepath.Dir(path), 0755)
}
return path
return filepath.Join(p.tempDir, fmt.Sprintf("%s-router.php", p.Version.Version))
Copy link
Member Author

@tucksaun tucksaun Feb 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Note for the reviewers: I moved the routing file to the same location as the FPM config.
This way I can always create and clean the directory no matter the SAPI

@tucksaun tucksaun requested a review from fabpot February 8, 2025 19:52
fabpot
fabpot reviewed Feb 9, 2025
View reviewed changes
Copy link
Contributor

@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

Regarding cleaning up existing empty directories, can we at least remove the ones that are older than x days?

@tucksaun
Copy link
Member Author

Regarding cleaning up existing empty directories, can we at least remove the ones that are older than x days?

good idea, will do

tucksaun and others added 2 commits February 14, 2025 05:54
@tucksaun @fabpot
Update local/php/executor.go
f7b227d 
Co-authored-by: Fabien Potencier <fabien@potencier.org>
@tucksaun
fix: cleanup stale temporary directories
e0b9e03 
please note that this might take a while because we clean by batches, use `find ~/.symfony5/tmp -type d -mindepth 1 -maxdepth 1 -delete` if you are eager
@tucksaun tucksaun requested a review from fabpot February 14, 2025 12:48
@tucksaun
Copy link
Member Author

@fabpot cleanup added. I considered directories as stale after 7 days. wdyt?

@fabpot fabpot merged commit 6004fc6 into symfony-cli:main Feb 14, 2025
1 check passed
@fabpot
Copy link
Contributor

fabpot commented Feb 14, 2025

Great! Thank you.

@tucksaun tucksaun deleted the fix/553 branch February 14, 2025 14:48
@srsbiz
Copy link

srsbiz commented Feb 15, 2025 •
edited
Loading

Thanks, unfortunately it is somehow not working for me. Are old files removed only on "clean" shutdown of the server? I cannot gracefully stop server with server:stop since #397 and while force shutting down with Ctrl+C there is some attempt to remove logs, but it almost always fail
image
Error message says that file is used by another process and cannot be accessed

@tucksaun
Copy link
Member Author

AFAICT this error is not related to this PR and is only related to #397 (on which I didn't had the opportunity to work so far)

@srsbiz
Copy link

srsbiz commented Feb 15, 2025

Yes, my question is not about this error, but on conditions required for cleanup process to be performed - if it requires "clean" shutdown by calling server:stop or if should be performed on any type of shutdown - like Ctrl+C, stopping process via Task Manager, issuing kill command with pid etc.

@tucksaun
Copy link
Member Author

On any shutdown. But your error happens during the shutdown process and things are not necessarily linear in Go so your error might end up stopping the cleaning process early on

@tucksaun
Copy link
Member Author

@srsbiz see #569 for an attempt to improve this for you (won't fix the initial error)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot Awaiting requested review from fabpot

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
3 participants
@tucksaun @fabpot @srsbiz