Use current time() instead of Request Time for the expiration TTL #85
Conversation
|
@MatthiasKuehneEllerhold the problem is clear, but a test (even using |
|
@Ocramius I've fixed the current unit-tests and added a new one. The problem with this kind of tests is that during the execution the second can switch between the calls. So we'd need to make them robust enough against that but they shouldn't take too long either. |
|
Travis fails on PHP 7.1 on this unit-test: |
|
@Ocramius Any feedback? |
|
Didn't get to check it at all, sorry: busy with other OSS projects atm
…On 22 Aug 2017 8:13 AM, "Matthias Kühne" ***@***.***> wrote:
@Ocramius <https://github.com/ocramius> Any feedback?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#85 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAJakAi5bAPZc133Xh5sioXiwIgsoLi2ks5sanGegaJpZM4O6OoE>
.
|
| $this->assertEquals($currentTimestamp + 3600, $metadata['EXPIRE']); | ||
| } | ||
|
|
||
| public function testSettingExpirationSecondsUsesCurrentTime() |
There was a problem hiding this comment.
Unfortunately, this test doesn't prove the fix.
To test, I reverted the change you provided (s/time()/$_SERVER['REQUEST_TIME']/), and ran just this test; it passed. I tried resetting the sleep and expiration seconds values as well, but they always passed. I also tried putting the assignment prior to the setExpirationSeconds() method, and that did not work, either.
While I do not doubt that the fix is correct, we need an accurate way to test it that emulates what happens in a normal server-side web request so that we can ensure the fix is correct and will not break with future updates.
|
Sorry for missing this! You were correct, the test proved nothing because the REQUEST_TIME never changed. I've fixed it and the REQUEST_TIME will now be temporarily overwritten to simulate a second request coming it. |
|
|
||
| // Simulate a second request, backup the request time and overwrite it with current time() | ||
| $requestTimeBackup = $_SERVER['REQUEST_TIME']; | ||
| $requestTimeFloatBackup = $_SERVER['REQUEST_TIME_FLOAT'] ?? null; |
There was a problem hiding this comment.
This library still supports PHP 5.6, so we'll need to do an isset() ? ... : null here. I can do that on merge, though.
| } catch (\Throwable $e) { | ||
| // Restore the original values | ||
| $_SERVER['REQUEST_TIME'] = $requestTimeBackup; | ||
| $_SERVER['REQUEST_TIME_FLOAT'] = $requestTimeFloatBackup; |
There was a problem hiding this comment.
I don't think we need to do this; PHPUnit restores superglobals between test executions by default, IIRC.
I'll try removing it locally when I merge.
|
I've verified the fix locally at this time by restoring the original code and running the new tests! Merging momentarily! |
|
Thanks! |
The calculation of the expiration timestamp is problematic if you have long-running requests (like user uploads). For long running requests the
$_SERVER['REQUEST_TIME']is far less than the current timestamp. For example uploading 500 MiB can take up to 4 hours in some of our use cases. We're usingsetExpirationSeconds()as a way to enforce a server-side inactivity timeout. Lets assume we're setting the expiration to 2 hours ($container->setExpirationSeconds(7200);) it will set the expiration timestamp to 2 hours from the START of the request ($_SERVER['REQUEST_TIME']) and not 2 hours from NOW (time()).Let me show this on an real-life example:
$container->setExpirationSeconds(7200)on every request.In order to fix this the container needs to use the current time() instead of the REQUEST_TIME for setting the new expiration TTL.
I hope I could make this (rather specific) problem clear.