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

Temp files processing refactoring #982

Merged
merged 3 commits into from
Nov 9, 2023
Merged

Conversation

luzrain
Copy link
Contributor

@luzrain luzrain commented Nov 5, 2023

I propose some changes in the way we work with temporary files during the execution of the "status" and "connections" commands.

Changes:

  • I have added tho new properties $processStatusFile and $connectionStatusFile instead of $statusFile.
  • Moved out initialization process of these properties to the init method.
  • Changed the default paths to the system temp directory.
  • Changed the way files are deleted after command execution. "connection" command didn't delete them at all.
  • Added formatConnectionStatusData() method similar to formatStatusData() but for "connections" command output (it's needed to add the ability to override the way the message is printed).
  • Print "connections" command output using the safeEcho() method.

Breaking changes:

  • $statusFile property removed, replaced with $processStatusFile and $connectionStatusFile.

Breaking changes in protected scope:

May only affect those who have overridden the Workerman class.

  • formatStatusData() method removed, replaced by formatProcessStatusData()
  • $statisticsFile property removed

Other changes:
Pid file default path also uses system temp directory by default.
Sleep time during "status" command execution reduced to 0.5sec.

@luzrain
Copy link
Contributor Author

luzrain commented Nov 6, 2023

One more thing I want to discuss. Do we even need the ability to set custom paths for status files since they are stored in the system temp directory by default? Maybe it's worth to removing $processStatusFile and $connectionStatusFile from the public scope at all? What do you think?

@luzrain
Copy link
Contributor Author

luzrain commented Nov 7, 2023

I've found the reason why the original temp directories were changed, so it might be better to save temp files in the old place. It's up to you.
#629
Personally, I think it is a bad idea to save any temp files in the vendor directory.

@walkor
Copy link
Owner

walkor commented Nov 7, 2023

Thank you very much for your PR.
I suggest keeping the statusFile so that old projects can be directly upgraded without making any changes.
The default value of pidFile is sys_get_temp_dir(), in Linux systems it is likely to be the /tmp directory, and many operating systems regularly clean up the /tmp directory, resulting in the loss of pid files.

@luzrain
Copy link
Contributor Author

luzrain commented Nov 7, 2023

I returned public $statusFile and changed back default system temp paths 👌

@walkor walkor merged commit 8c5ce5e into walkor:master Nov 9, 2023
18 checks passed
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

Successfully merging this pull request may close these issues.

2 participants