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

Install tool uses same names for .lock files in all installations #1107

Closed
tonsinn opened this issue Sep 26, 2017 · 17 comments
Closed

Install tool uses same names for .lock files in all installations #1107

tonsinn opened this issue Sep 26, 2017 · 17 comments
Assignees
Labels
Milestone

Comments

@tonsinn
Copy link

tonsinn commented Sep 26, 2017

Running multiple Contao 4.4.5 websites on the same dedicated server (all-inkl) allows the call of the install tool for one installation only until the root user deletes all .lock files in the global /tmp directory. Even a modification of the PHP session path in .htaccess solves not the problem.

Another impact is, that in Maintenance a „Recreate the symlinks“ brings in all Contao installations the same server error:

fopen(/tmp/sf.contao-symlinks.59a5ad6d1e94fd76fb1f72c544f6e14ac0dfe26f0441f5e81b370a382e46e65a.lock): failed to open stream: Permission denied

The comment from the server admin is: “Es sollten variable Dateibezeichnungen vom Install-Tool verwendet und auch diese Dateien am Ende wieder entfernt werden. Die identischen Namen für die .lock-Dateien verursachen ja erst die Probleme.” I agree. What could be any reason for identical names for the .lock files and why do they remain after finishing the install tool?

On a dedicated server at domainFactory this problem does also occur with multiple 4.4.5 installations by different SSH-users but one global /tmp. Even after changing session.save_path to a local directory in PHP.INI only the first opend install tool of in the whole server works.

@aschempp
Copy link
Member

aschempp commented Sep 26, 2017 via email

@Toflar
Copy link
Member

Toflar commented Sep 26, 2017

Multiple comments here:

  1. It's not related to the install tool at all, the install tool is just one place where this might happen.
  2. We don't use the "Symfony locking component". Symfony will introduce a lock component in version 3.4 so just to prevent any confusion: we don't use this (of course, as it's not released yet) but instead use the Symfony LockHandler.
  3. The LockHandler does not generate a random name but instead always a SHA256 version of the lock name. Of course, otherwise the whole idea of locking would get lost 😄
  4. The LockHandler accepts a directory to which it writes the locks and falls back to system_get_tmp_dir() if you don't provide one. And that's the issue. We should provide a path that is unique to the application, not the system (even though they're the same in best case).

I think we should adjust the AbstractLockedCommand to look something like this:

final protected function execute(InputInterface $input, OutputInterface $output)
{
-    $lock = new LockHandler($this->getName());
+    $projectDir = $this->getContainer()->getParameter('kernel.project_dir');
+    $lockDir = rtrim($projectDir, '/') . '/var/contao-command-locks';
+    $lock = new LockHandler($this->getName(), $lockDir);

@leofeyer leofeyer added the bug label Sep 26, 2017
@leofeyer leofeyer added this to the 4.4.6 milestone Sep 26, 2017
@tonsinn
Copy link
Author

tonsinn commented Sep 26, 2017

Yes, setting sys_temp_dir in PHP.INI to a individual value solves the problem.

@tonsinn tonsinn closed this as completed Sep 26, 2017
@dmolineus
Copy link
Contributor

I think Contao should use an unique path by default

@leofeyer leofeyer reopened this Sep 26, 2017
@leofeyer
Copy link
Member

I think so, too.

@leofeyer
Copy link
Member

-    $lock = new LockHandler($this->getName());
+    $projectDir = $this->getContainer()->getParameter('kernel.project_dir');
+    $lockDir = rtrim($projectDir, '/') . '/var/contao-command-locks';
+    $lock = new LockHandler($this->getName(), $lockDir);

+1 for the idea, however, this will only work if symfony-var-dir is set to var. Don't know if $this->getContainer()->getParameter('kernel.cache_dir').'/../../locks' is any better?

@leofeyer
Copy link
Member

Why don't we use the cache_dir for the lock files, too?

@Toflar
Copy link
Member

Toflar commented Sep 27, 2017

A cache dir should never contain temporary data. See discussion in symfony/symfony#23354 but for now there seems to be no official decision on the Symfony side.

@leofeyer
Copy link
Member

Maybe we should re-add a temporary directory? We still have system/tmp. 😄

@Toflar
Copy link
Member

Toflar commented Sep 27, 2017

@leofeyer
Copy link
Member

I just checked and the contao.cache (Doctrine\Common\Cache\FilesystemCache) stores its data in "%kernel.cache_dir%/contao/cache", so I think we can safely use this for the lock handler as well, don't you agree?

@Toflar
Copy link
Member

Toflar commented Sep 27, 2017

I agree, yes. We can still move all our tmp data once there is such a concept in SF :)

@leofeyer
Copy link
Member

Fixed in e57da33.

@aschempp
Copy link
Member

Just as a side note, the cache folder is not necessarily supposed to be writable. I've read up on the topic of that recently and Symfony recommended to do store stuff in var, but just create folders like var/data or var/uploads or whatever your app requires. I agree though that var/cache is our current best options.

@DanielSchwiperich
Copy link
Contributor

One problem I see or encountered with this is, if you install contao via cli and using a different os user then when accessed via webserver it throws errors because the generated folder in /tmp is not writeable for contao anymore. for example if you upload files.

@leofeyer
Copy link
Member

Symfony recommended to do store stuff in var, but just create folders like var/data or var/uploads

I know and I have also discussed this with @Toflar and @ausi. The problem, however, is that there is no kernel.var_dir in Symfony, so I cannot determine the path to create the new folders in.

I agree though that var/cache is our current best options.

That's not what we have agreed on though. 😄

sys_get_temp_dir().'/'.md5($this->getContainer()->getParameter('kernel.project_dir'))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants