Error handling
Closed this issue · 8 comments
We are using phpiredis on a high-traffic site and are quite impressed with its performance and ease of use. One thing thats missing though is proper error handling. Currently, when the server replies with an error message (for example when a command is issued against a key of the wrong type), phpiredis will raise a PHP error, which must be handled by the user application.
Unfortunately PHP does not offer any form of classification for errors, all errors must be handled by the same error handler function. This makes it very hard to properly encapsulate the Redis functionality.
We would like to handle the errors coming out of phpiredis separately from other errors that may be happening elsewhere. I can think of three possible strategies for that, all of which don't raise PHP errors:
- Throw exceptions. Quite clean and very flexible - the user application can freely decide which exceptions to catch and handle (or not catch at all), regardless of what other errors may occur. This would however require the introduction of an Exception class, which may not be desirable.
- Store the error internally and return a special value indicating failure. The error would be made accessible through a function like
phpiredis_last_error(). This special return value could be eitherfalseornull. As these values can also be valid return values, the user application, upon seeing the value, should call the error accessor and determine whether an error really occurred. (This is how phpredis does it) - Allow a custom error handler to be injected via a function. This would also be very favorable, as it gives total control over how errors are handled. An error handler could be set globally for the extension or on a per-connection basis. In the case of an error, this error handler would get called and the function would return
false.- There is already the possibility to create a
phpiredis_readerfrom PHP code, and a function to set an error handler on a reader also exists. I could however not find a way of tellingphpredis_command_bs()for example, to use this specific reader. Is there a way of doing this? It seems like these readers are never used.
- There is already the possibility to create a
I am willing to implement the option that is decided on, and provide it via a pull request. However, I wanted to first open an issue to provide the possibility for a discussion.
This would be great for us too. The first option is most standard, and for backwards compatibility could be enabled by setting a constant or specifying the desired behavior during initialization.
I like both 1 and 3 but I'd be more inclined to favor 3 since it's the same approach we used for phpiredis_reader. Currently the client and the reader resources are completely separate and don't share any kind of interaction, this is due to the fact that the reader bits were implemented at a later stage to make this library useful for other pure-PHP libraries such as Predis to optionally speed things up a bit with protocol parsing and serialization.
The advantage of 3 is that users can customize the behavior of the client simply by supplying a callable object to make it raise a PHP error (like it happens currently when using the client), return a custom error object (like it happens with Predis that uses only the reader resource) or simply throw an exception (which is the point of the current discussion). We could also make the client raise a PHP error by default to retain the current behavior when there's no custom handler specified, but maybe we should just aim to introduce breaking changes altogether (see later in my comment). I'm not sure about having a distinction between global or per-connection handlers, I'd go only with the second but maybe I'm missing a use case that would make the global ones useful.
But while this is cool to handle errors returned by Redis, it doesn't solve our problem with plain connection errors (e.g. the server goes down while doing stuff)...
Slightly related: I was meaning to finally freeze the library and tag a 1.0.0 release to start making changes (even breaking ones) targeting a new major release sometime in the future. The change described in your issue would be a good start in this direction, and I have some more ideas that I was planning to share in a issue open for discussion. Unfortunately my C skills are not something to be proud of and also a bit rusty since the last time I used it extensively was in high-school (quite a while ago :-)) so literally any kind of help and contribution would be more than appreciated.
Hi,
I'm with some free time at the moment, so I can take a stab at this, or review @theintz's implementation but I agree on deciding an interface first.
About implementations, I prefer option 3, adding a two new optional arguments to phpiredis_{multi_}command{,_bs} that would be called on error and on status and return whatever those functions return.
The phpiredis_reader interface might also be used instead and only use one extra new optional argument, but I think most of the users won't prefer to initialize a reader for these functions.
So the debate swings in favor of setting an error handler. However, instead of passing the handler(s) each time a phpiredis function is called, I would favor adding a function phpiredis_set_error_handler(), which attaches a custom error handler to a connection. This error handler could also be used when a connection error happens. Maybe we could define a few constants that represent the type of error: PHPIREDIS_CONNECTION_ERROR and PHPIREDIS_PROTOCOL_ERROR, for example.
@theintz I agree with you and that's what I was referring to when I said that we should use per-connection handlers.
I like @seppo0010's suggestion of using the same resource returned by phpiredis_reader_create() as an optional argument to set custom status and error handlers used to process Redis responses, but instead of passing it each time a function is called I'd pass it only once when we create the connection. The function would look something like this:
phpiredis_connect($host, $port, $reader = null);As for connection or protocol parsing errors we can indeed add a new function with a signature like phpiredis_set_error_handler($connection, $handler) where $handler is a callable with a signature like function($message, $type) (and where $type is populated with the values of PHPIREDIS_CONNECTION_ERROR or PHPIREDIS_PROTOCOL_ERROR depending on the error)
What I don't know is how that works considering the connection might be persistent.
@seppo0010 For all PHP knows, there are no persistent connections. For every request, the connection must be re-initialized by calling phpiredis_pconnect() anyways (in order to get the connection handle), so it might as well be non-persistent (connections are however cached/persisted inside the extension code). The error handler then has to be set on the connection for every request as well.
I am going to get started on adding the function and the constants we talked about, I have to admit however that C is not my native language either. Much of the code for setting an error handler is already in the extension though, so it should not be very difficult. Will create a pull request when I have it working.
In the meanwhile I'm going to create a new v1.0 branch and tag v1.0.0 soon, the new developments regarding this issue will be eventually available in a new major release.