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

Fix LDAP race conditions #24052

Merged
merged 1 commit into from
Apr 25, 2016
Merged

Conversation

MorrisJobke
Copy link
Contributor

  • getFromCache is wrapped in isCached
  • inbetween the two calls the cache entry hits it's TTL
  • getFromCache returns null
  • this fix only checkes if the returned value is null and
    return only non-null values

cc @blizzz @nickvergessen @LukasReschke

return $this->access->connection->getFromCache($cacheKey);
$inGroup = $this->access->connection->getFromCache($cacheKey);
if(!is_null($inGroup)) {
return $inGroup;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do make static code scanners happy like Scrutinizer can you cast this to bool?

* getFromCache is wrapped in isCached
* inbetween the two calls the cache entry hits it's TTL
* getFromCache returns null
* this fix only checkes if the returned value is null and
  return only non-null values
@MorrisJobke
Copy link
Contributor Author

@LukasReschke I addressed your comments :)

@MorrisJobke
Copy link
Contributor Author

@LukasReschke @nickvergessen @blizzz Could we get this tested and merged? I really would like to have this in place.

@davitol @owncloud/qa @owncloud/ldap We need some LDAP testing here.

@MorrisJobke
Copy link
Contributor Author

@karlitschek I would like to request a backport of this down to stable8.2 for this race condition.

@MorrisJobke
Copy link
Contributor Author

This fixes race conditions, that are really hard to reproduce. The testing here just involves a verification, that the LDAP connection still works.

@karlitschek
Copy link
Contributor

nice. please backport 👍

@davitol
Copy link
Contributor

davitol commented Apr 25, 2016

👍 WFM. Configuring a LDAP and a AD no problems were found

@davitol davitol added the tested label Apr 25, 2016
@MorrisJobke
Copy link
Contributor Author

I also got positive feedback.

@MorrisJobke
Copy link
Contributor Author

stable9: #24242
stable8.2: #24243

@DeepDiver1975 DeepDiver1975 merged commit 6321596 into master Apr 25, 2016
@DeepDiver1975 DeepDiver1975 deleted the fix-ldap-cache-race-conditions branch April 25, 2016 12:55
@MorrisJobke MorrisJobke removed their assignment Apr 25, 2016
@blizzz
Copy link
Contributor

blizzz commented Apr 25, 2016

Integration tests still succeed 👍

@lock
Copy link

lock bot commented Aug 6, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants