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

[HttpKernel] Lookup the response even if the lock was released after two second wait #16274

Merged
merged 1 commit into from Jan 25, 2016

Conversation

jakzal
Copy link
Contributor

@jakzal jakzal commented Oct 18, 2015

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

While looking into #15813 I noticed that we wait for the lock to be released for five seconds, but then only do a lookup if the lock was released in two seconds, no more.

I think it's worth to make both values the same (so either two or five seconds). I see no reason why we should wait for the lock for five seconds, but then only do a lookup if we waited for two. One way the wait either takes too long, the other way we loose the opportunity to actually return a response.

@fabpot
Copy link
Member

fabpot commented Jan 25, 2016

Thank you @jakzal.

@fabpot fabpot merged commit 9963170 into symfony:2.3 Jan 25, 2016
fabpot added a commit that referenced this pull request Jan 25, 2016
…ased after two second wait (jakzal)

This PR was merged into the 2.3 branch.

Discussion
----------

[HttpKernel] Lookup the response even if the lock was released after two second wait

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

While looking into #15813 I noticed that we [wait for the lock to be released for five seconds, but then only do a lookup if the lock was released in two seconds](https://github.com/symfony/symfony/blob/fa604d3c6f16f264863a42c200391ab996640296/src/Symfony/Component/HttpKernel/HttpCache/HttpCache.php#L540-L562), no more.

I think it's worth to make both values the same (so either two or five seconds). I see no reason why we should wait for the lock for five seconds, but then only do a lookup if we waited for two. One way the wait either takes too long, the other way we loose the opportunity to actually return a response.

Commits
-------

9963170 [HttpKernel] Lookup the response even if the lock was released after 2 seconds
@jakzal jakzal deleted the http-cache-wait-for-lock branch January 25, 2016 14:11
@fabpot fabpot mentioned this pull request Feb 3, 2016
@patrick-mcdougle
Copy link
Contributor

Should this value be an option in the options array instead of a magic number?

This was referenced Feb 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants