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

always refresh cron.txt #8838

Merged
merged 3 commits into from Jan 19, 2018

Conversation

Projects
None yet
3 participants
@binfalse
Copy link
Contributor

binfalse commented Jan 12, 2018

cron.txt contains the timestamp of the last cron run. The file is only updated with the current timestamp once cron is actually executed.
However, with Docker use cases you may throw away a container any time and start from a clean Contao instance (eg in case of updates, migrations etc), only mounting/linking persistent stuff into the container.
Therefore, the next cron run may only be in a few hours (database still knows lastrun), and the cron.txt is not created...
Thus, every browser request will always entail an ugly 404, until the next cron run...

This patch will always write the cron.txt containing:

  • the current timestamp, if cron is really executed
  • the timestamp from the database, otherwise

Writing the cron.txt even if it already exists shouldn't be a big deal..?

See also Docker image at:
https://hub.docker.com/r/binfalse/contao/

always refresh cron.txt
cron.txt contains the timestamp of the last cron run. the file is only
updated with the current timestamp once cron is actually executed.
however, with docker use cases you may throw away a container any time
and start from a clean contao instance (eg for updates, migrations etc),
only mounting/linking persistant stuff into the container.
therefore, the next cron run may only be in a few hours (database still
knows lastrun), thus the cron.txt is not created...
thus, every browser request will also entail an ugly 404...

this patch will always write the cron.txt containing:

* the current timestamp, if cron is really executed
* the timestamp from the database, otherwise

writing the cron.txt even if it already exists shouldn't be a big deal..?

see also docker image at:
https://hub.docker.com/r/binfalse/contao/
@aschempp
Copy link
Contributor

aschempp left a comment

This will cause a file write operation on every access, but I think that's fine. But we should move the line out of the if-else blocks then (just next to the unlockTables).

@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Jan 15, 2018

This will cause a file write operation on every access, but I think that's fine.

Well I don't.

@binfalse

This comment has been minimized.

Copy link
Contributor

binfalse commented Jan 15, 2018

The update on cron.txt will now be written after unlocking the cron table, not matter if it gets a new or an old value.

@leofeyer leofeyer closed this Jan 15, 2018

@leofeyer leofeyer changed the base branch from master to 3.5 Jan 15, 2018

@leofeyer leofeyer reopened this Jan 15, 2018

@binfalse

This comment has been minimized.

Copy link
Contributor

binfalse commented Jan 16, 2018

@leofeyer that means you're not convinced, yet?
Do you think that is too much overhead to write the cron.txt? Or too much io load?

I mean, it will only be written max once per minute - even on very busy contao instances with hundreds of cron jobs.. And dumping a simple number once per minute into a file shouldn't be too much work for a web server...? I mean for that very same request, the webserver typically writes ways more logs just for the request (and usually you have tons more requests before arriving at cron.php...)
Or are there any other concerns?

What do you think about reverting eacb91d again and instead keep the else from 754d366 and add a check if the file already exists? Something like:

// Otherwise make sure cron.txt contains the correct lastrun-time
else
{
	if (!file_exists ('system/cron/cron.txt'))
		$this->updateCronTxt($objCron->value);
}

However, with eacb91d the cron table won't be blocked while writing cron.txt

@leofeyer leofeyer added defect and removed up for discussion labels Jan 18, 2018

@leofeyer leofeyer added this to the 3.5.33 milestone Jan 18, 2018

@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Jan 18, 2018

As discussed in Mumble on January 18th, 2018, we want to merge this PR.

@binfalse

This comment has been minimized.

Copy link
Contributor

binfalse commented Jan 18, 2018

Cool. Let me know if I can do anything! :)

@leofeyer leofeyer merged commit 49679df into contao:3.5 Jan 19, 2018

@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Jan 19, 2018

Thank you @binfalse.

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