Hi!
Recently it seems as though there's been a huge increase in the use of exceptions within the MediaWiki ecosystem. That's perfectly fine. Exceptions are fantastic and a standard part of any developer's toolkit.
However, there seems to be a trend in throwing exceptions for codepaths that don't catch them prior to returning to the user. The one I'm calling out in particular is InvalidArgumentException.
It seems as though some code paths that are relying on user-generated input are throwing this exception. That's pretty evil. There's a couple of reasons: - This just returns a generic 503 error to our users. They don't know what went wrong and they're left wondering why and filing bugs. - Due to our current logging setup, we don't do nice parameter grouping for exceptions. This means that each one is reported on its own, and we don't have an idea of the scale/severity of the problem. We need to fix this, but it doesn't preclude us being more proactive.
Basically the short version is: exceptions should only be shown to users in the situation of *actual software errors*. They're the exception, not the norm. What we *should* do in such situation is log the error (at the ERROR/WARNING/etc level as appropriate) and then gracefully fall back.
I don't think we need an RfC or anything formal here, just a little bit of common sense :)
Tldr: if there's no such user "Foo", we shouldn't return an exception when a user tries to perform an action on it. We should return a helpful error message, log it, and fall back gracefully.
-Chad
PS: Now that I think about it, there's been a proliferation of RuntimeException for the exact same thing.
Am 07.12.2017 um 19:48 schrieb Chad:
Basically the short version is: exceptions should only be shown to users in the situation of *actual software errors*. They're the exception, not the norm. What we *should* do in such situation is log the error (at the ERROR/WARNING/etc level as appropriate) and then gracefully fall back.
I agree with the idea, but I'd like to point out that log-and-fall-back should *not* be the normal way to handle things, IMHO.
Let's take for example an invalid title. If a user supplied an invalid title, they should get a helpful error. There is no reason to even log this, really. This should be done by code that expects to handle user input, and can involve throwing and catching exceptions, or be handled some other way.
Code that does not expect raw user input usually SHOULD thrown an InvalidArgumentException if it gets invalid input, though! Something went wrong, so we should fail fast & safe!
We should however improve how we show exceptions to users. We have "nicer" handling for MWException than for other exceptions. I can think of no good reason for this distinction - can't we do the nicer handling for all exceptions?
The log-and-fall-back case should be quite rare, and be reserved for compatibility code - compatibility with old data, in particular. So perhaps the invalid title comes from the page or the pagelinks table - that's unexpected, but not impossible: someone may have fiddled with $wgLegalTitleChars, rendering once-valid titles invalid. So in that case, log-and-fall-back is the correct behavior.
On Thu, Dec 7, 2017 at 11:07 AM Daniel Kinzler daniel.kinzler@wikimedia.de wrote:
Am 07.12.2017 um 19:48 schrieb Chad:
Basically the short version is: exceptions should only be shown to users
in
the situation of *actual software errors*. They're the exception, not the norm. What we *should* do in such situation is log the error (at the ERROR/WARNING/etc level as appropriate) and then gracefully fall back.
I agree with the idea, but I'd like to point out that log-and-fall-back should *not* be the normal way to handle things, IMHO.
Let's take for example an invalid title. If a user supplied an invalid title, they should get a helpful error. There is no reason to even log this, really. This should be done by code that expects to handle user input, and can involve throwing and catching exceptions, or be handled some other way.
We're on the same page here. It doesn't inherently need to be logged, that's why I qualified it with "as appropriate." If it's something where the user just typed something bogus, we should return a helpful error to them and move on. And the emphasis in what you say here *really* needs to be on CATCHING the exception. Throwing with no catch is what's evil :)
Code that does not expect raw user input usually SHOULD thrown an InvalidArgumentException if it gets invalid input, though! Something went wrong, so we should fail fast & safe!
Indeed, I have no qualms with this. The point I'm making is on user-generated actions.
We should however improve how we show exceptions to users. We have "nicer" handling for MWException than for other exceptions. I can think of no good reason for this distinction - can't we do the nicer handling for all exceptions?
Because our exception handling code is crap. Nobody's had the cycles to work on it. Volunteers welcome ;-)
MWException is *very* heavy weight, and sadly doesn't let us use native PHP extensions that make more sense. InvalidArgumentException is descriptive, MWException is not. And a MWInvalidArgumentException that extends MWException just to be descriptive seems overkill.
In an ideal world: we don't need MWException at all, or else it becomes a lightweight wrapper exception for any other type on demand. I'm thinking something like
throw MWException( 'InvalidArgumentException' )
Who knows?
The log-and-fall-back case should be quite rare, and be reserved for compatibility code - compatibility with old data, in particular. So perhaps the invalid title comes from the page or the pagelinks table - that's unexpected, but not impossible: someone may have fiddled with $wgLegalTitleChars, rendering once-valid titles invalid. So in that case, log-and-fall-back is the correct behavior.
Indeed. Not all bad behavior needs logging (or sometimes, just at a DEBUG/INFO level because data is nice but yelling at log watchers is not).
Details all tbd, but I don't think you and I disagree here much at all :)
-Chad
For "expected" exceptions, you can throw ErrorPageError or one of its subclasses, which is handled internally to produce a pretty user-facing error page.
On Thu, Dec 7, 2017 at 11:15 AM Bartosz Dziewoński matma.rex@gmail.com wrote:
For "expected" exceptions, you can throw ErrorPageError or one of its subclasses, which is handled internally to produce a pretty user-facing error page.
Yeah, and when you throw ErrorPageError deep enough in a code path, it can explode on cli operations. I've seen this before.
Our MWException handling is weird :\
-Chad
On 2017-12-07 21:05, Chad wrote:
Yeah, and when you throw ErrorPageError deep enough in a code path, it can explode on cli operations. I've seen this before.
Indeed you did, you even fixed this bug a couple months ago ;) I'm pretty sure it's safe to use now.
25c3c061b51cbfe377ebb2decbe09f7db7bc7397 https://gerrit.wikimedia.org/r/#/c/363660/
On Thu, Dec 7, 2017 at 12:24 PM Bartosz Dziewoński matma.rex@gmail.com wrote:
On 2017-12-07 21:05, Chad wrote:
Yeah, and when you throw ErrorPageError deep enough in a code path, it
can
explode on cli operations. I've seen this before.
Indeed you did, you even fixed this bug a couple months ago ;) I'm pretty sure it's safe to use now.
25c3c061b51cbfe377ebb2decbe09f7db7bc7397 https://gerrit.wikimedia.org/r/#/c/363660/
Hacked so as to not explode. But the whole thing is a mess still.
-Chad
wikitech-l@lists.wikimedia.org