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

Catch Exceptions thrown during authentication #35

Merged
merged 4 commits into from
Nov 25, 2015
Merged

Conversation

danielbachhuber
Copy link
Contributor

This will let us more easily catch an Exception if there's an error
connecting.

Fixes #34

This will let us more easily catch an Exception if there's an error
connecting.
@danielbachhuber danielbachhuber added this to the v0.2.2 milestone Nov 17, 2015
@danielbachhuber
Copy link
Contributor Author

@jtsternberg Can you try this out on your site with authentication and confirm it catches your Exception? I don't have an easy way of writing tests for the authentication case.

@danielbachhuber danielbachhuber self-assigned this Nov 17, 2015
@danielbachhuber
Copy link
Contributor Author

Actually, I take that back. Once #40 is merged, I can write a test for failed authentication.

@jtsternberg
Copy link

Changing to this actually caused my error log to explode w/ these errors (repetitively):

[18-Nov-2015 15:38:17 UTC] WP Redis: Failed to AUTH connection
[18-Nov-2015 15:38:17 UTC] WP Redis: Failed to AUTH connection
[18-Nov-2015 15:38:17 UTC] WP Redis: Failed to AUTH connection
[18-Nov-2015 15:38:17 UTC] WP Redis: Failed to AUTH connection
[18-Nov-2015 15:38:17 UTC] PHP Warning:  WP Redis: read error on connection in D:\home\site\wwwroot\wp-content\plugins\wp-redis\object-cache.php on line 741

@danielbachhuber
Copy link
Contributor Author

Changing to this actually caused my error log to explode w/ these errors (repetitively):

Ah, because it tries to reconnect repeatedly.

@jtsternberg
Copy link

Actually, it may have to do w/ my tweaks to the file. I think the issue is that this error ("Failed to AUTH connection") is not in the $retry_exception_messages array, so it doesn't actually try to reconnect.

@jtsternberg
Copy link

Ok, it most definitely has to do with $this->is_redis_connected not yet being set when $this->_call_redis( 'auth', $redis_server['auth'] ); is called. I think $this->is_redis_connected has to be set before AND after the auth call. (like so: http://b.ustin.co/XV5i)

Rather than trying to use our fallback mechanism, we can just trigger an
error here and let the fallback mechanism kick in later.
@danielbachhuber danielbachhuber changed the title Make authentication calls with _call_redis() Catch Exceptions thrown during authentication Nov 25, 2015
@danielbachhuber
Copy link
Contributor Author

Failing test case looks like this:

salty-wordpress ➜  wp-redis  phpunit --filter test_redis_bad_authentication
Installing...
Running as single site... To run multisite, use -c tests/phpunit/multisite.xml
Not running ajax tests. To execute these, use --group ajax.
Not running ms-files tests. To execute these, use --group ms-files.
Not running external-http tests. To execute these, use --group external-http.
PHPUnit 4.4.1 by Sebastian Bergmann.

Configuration read from /srv/www/pantheon.dev/wp-content/plugins/wp-redis/phpunit.xml

E

Time: 2.31 seconds, Memory: 23.00Mb

There was 1 error:

1) CacheTest::test_redis_bad_authentication
RedisException: Redis server went away

danielbachhuber added a commit that referenced this pull request Nov 25, 2015
Catch Exceptions thrown during authentication
@danielbachhuber danielbachhuber merged commit 3544fd2 into master Nov 25, 2015
@danielbachhuber danielbachhuber deleted the 34-safe-auth branch November 25, 2015 00:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants