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

[HttpKernel] reset kernel start time on reboot #27344

Merged
merged 1 commit into from May 25, 2018

Conversation

Projects
None yet
5 participants
@kiler129
Contributor

kiler129 commented May 23, 2018

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #27319
License MIT
Doc PR n/a

I created branch from 3.4, since the furthest thing I could find for the reboot feature was a4fc492 and it originated during stabilization phase of 3.4.

ping @nicolas-grekas

@@ -111,6 +111,10 @@ public function __clone()
public function boot()
{
if (true === $this->booted) {
if ($this->debug) {
$this->startTime = microtime(true);
}

This comment has been minimized.

@fabpot

fabpot May 23, 2018

Member

I would remove the initialization done currently in __clone() as this is now redundant with this one.

Also, to be consistent, what about removing the same code in __construct() and always execute the initialization when the boot() method is called?

This comment has been minimized.

@fabpot

fabpot May 23, 2018

Member

As a side note, there is a side effect of this change (but this was already the case as of 3.4). Before 3.4, calling boot() a second time was a no-op. As of 3.4, calling boot() a second time re-initialize the Kernel.

This comment has been minimized.

@kiler129

kiler129 May 23, 2018

Contributor

@fabpot I fully agree with the first part about removing it from __clone().

My only concern is removing the time generation from __construct() and moving it totally to boot(). In standard installation I would say it's extremely unlikely that someone used the time before boot(), but should we care about possible edge-case here?

This comment has been minimized.

@fabpot

fabpot May 23, 2018

Member

No, I would say that it does not matter (that is internal stuff anyway). And that gives more consistent numbers between the first request and the following ones.

This comment has been minimized.

@kiler129

kiler129 May 24, 2018

Contributor

@fabpot Fixed.

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone May 23, 2018

@nicolas-grekas nicolas-grekas changed the title from [HttpKernel] fix bug #27319 reset kernel start time on reboot to [HttpKernel] fix reset kernel start time on reboot May 25, 2018

@nicolas-grekas nicolas-grekas changed the title from [HttpKernel] fix reset kernel start time on reboot to [HttpKernel] reset kernel start time on reboot May 25, 2018

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented May 25, 2018

Thank you @kiler129.

@nicolas-grekas nicolas-grekas merged commit b7feef0 into symfony:3.4 May 25, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request May 25, 2018

bug #27344 [HttpKernel] reset kernel start time on reboot (kiler129)
This PR was squashed before being merged into the 3.4 branch (closes #27344).

Discussion
----------

[HttpKernel] reset kernel start time on reboot

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #27319
| License       | MIT
| Doc PR        | n/a

I created branch from 3.4, since the furthest thing I could find for the reboot feature was a4fc492 and it originated during stabilization phase of 3.4.

ping @nicolas-grekas

Commits
-------

b7feef0 [HttpKernel] reset kernel start time on reboot
@@ -110,6 +102,10 @@ public function __clone()
*/
public function boot()
{
if ($this->debug) {
$this->startTime = microtime(true);

This comment has been minimized.

@stof

stof May 25, 2018

Member

shouldn't we do this only when we actually do something (i.e. when not booted, or when the booting again without a request stack size) ?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas May 25, 2018

Member

done in 9de5014

@fabpot fabpot referenced this pull request May 26, 2018

Merged

Release v4.1.0-BETA3 #27390

This was referenced Jun 25, 2018

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