A little light (not entirely) WTF for a WTF codebase

Arantor

I put this into the codebase at work this week.

try {
   // stuff goes here
} catch (\Exception $e) { /* watch me care if this fails */ }

Discuss.

swayde

@Arantor said in A little light (not entirely) WTF for a WTF codebase:

I put this into the codebase at work this week.
try {
   // stuff goes here
} catch (\Exception $e) { /* watch me care if this fails */ }
Discuss.

Unless you have a really good reason to do that, it's stupid as fuck.
But you don't seem to be the kind of person to fuck it up intentionally, or be stupid.
So I'd like to know why you did it.

Arantor

@swayde OK, so here's the scenario.

I have some data processing that runs before this, collecting mandatory data. This particular chunk of code handles getting additional metadata to attach to the mandatory stuff.

In this case, fetching an image that can be attached to the data, which can fail for a variety of reasons - URL not found, image invalid format, over sized etc.

The code that is called, is a bunch of helper classes that all throw out exceptions for various failure states, and for the times it matters (e.g. response to user), we can actually catch the different types of exceptions and feed it back to the user.

This is a case though where it shouldn't fail (because it's been pre-vetted earlier in workflow) and if it does, the spec outright says it doesn't matter and we can ignore reporting on it, or acting on it.

It's one of the rare times when I do not need to care if it fails, or if it does, how it fails. Ordinarily I care, this time I don't, but to prevent the entire thing killing a batch processing run, I swallow the exception intentionally because in that situation it doesn't matter.

swayde

@Arantor completely sane (as expected). I'd add that explanation to the code, just to make sure the next monkey knows why it's like this.

asdf

@Arantor Why not catch Throwable while you're at it?

cartman82

@Arantor said in A little light (not entirely) WTF for a WTF codebase:

This is a case though where it shouldn't fail (because it's been pre-vetted earlier in workflow) and if it does, the spec outright says it doesn't matter and we can ignore reporting on it, or acting on it.

If it shouldn't fail, why not just leave out the try/catch block entirely? That way if it DOES fail, you catch a potential DB corruption of whatever unexpected condition caused it quietly, instead of showing scary looking messages to end users?

Arantor

@cartman82 Because 'shouldn't' isn't 'doesn't'.

The same code could conceivably fail. I just don't care if it does.

While it's doing some URL stuff, it's also doing thumbnail generation. There are times this can legitimately fail and I do not care if it does - and it's even expected. Normally, this code is related to pushing images or PDFs around, for which we can generate thumbnails. But if the user uploads, say, a Powerpoint file, we can't make a thumbnail of it. We don't care if we don't have it. It's just 'trying to regenerate thumbnail' will fail because it can't make it.

We're talking framework level code being reused here and sometimes it will fail, there's a whole bunch of reasons and in this particular batch processing case, IDGAF if it hasn't got a thumbnail ;)

DogsB

@Arantor I've done that a couple of times in my current stint. Mostly because if something does go wrong there is fuck all that can really be done about it. I just throw a runtime exception and let it float up. The batch job fails and I go investigate why the stars haven't aligned. There is a refactor on the way but since we can't say no to client requests (not for any business reason but rather a patlogical need to be liked by them ...) our background shit is going to be shit for a while.

Bad practice yes but reality likes to kick you in the balls.

Also post more often. I miss you :(

Kian

@Arantor said in A little light (not entirely) WTF for a WTF codebase:

I put this into the codebase at work this week.
try {
   // stuff goes here
} catch (\Exception $e) { /* watch me care if this fails */ }
Discuss.

Proposed improvement:

/* try {
  // stuff goes here
} catch (\Exception $e) {} */

You don't care if it fails so why even do it? Assume it always fails, improve performance! Otherwise, if the failure is expected (generating a thumbnail of something you know can't have thumbnails) prevent the failure with a check that tells you if it can succeed or not, then don't do it if you know you can't.

dse

Pass a fail=False to individual submodules called in the batch job, so that they will do their best not to fail and if they still fail well there is something wrong.
a. This will push swalloing the exceptions as close to the source of exception as possible, where individual modules know better.
b. If really unexpected exceptions happen like alien invasion there will be a record for future humanity
At least log the exception
Keep the exceptions+stack traces in a list, and at the end of the batch job print unique set of them

cartman82

@dse said in A little light (not entirely) WTF for a WTF codebase:

Pass a fail=False to individual submodules called in the batch job, so that they will do their best not to fail and if they still fail well there is something wrong.
a. This will push swalloing the exceptions as close to the source of exception as possible, where individual modules know better.
b. If really unexpected exceptions happen like alien invasion there will be a record for future humanity

This sounds wonky and like a bad practice. You don't want to add such flimsy code forks.

Better solution would be to just find all the exception that are expected to be thrown and swallow just them.

At least log the exception

Meaningless if no one ever looks at the logs. And pointless if he keeps getting bombarded by error alerts he doesn't care about.

Keep the exceptions+stack traces in a list, and at the end of the batch job print them

Better.

Or fill in some list/table and send like a weekly report. Just for sanity's sake.

tufty

As Kian says, you need to know in advance whether the action (in this case thumbnailing) can succeed.

I suggest setting up a webservice on a separate TLD, pass the data over to it and get back a boolean encoding of whether the action should succeed, along with a serialisation of the stub code reuired to call the relevant thumbnailing function (if not a serialisation of the actual thumbnailing function itself).

If you get a FALSE, NULL or BOOOL_UNSURE, call the stub code just in case, but be ready to catch and ignore any exceptions.

Arantor

@Kian While generally I'd agree, this is part of an ongoing effort to bring some kind of sanity to the codebase.

Essentially, the stuff is two or sometimes three calls to parts of the framework, as opposed to the 50+ lines, copy/pasted throughout the application and in pretty much all the other cases where we do those operations I want to catch the errors in a way where I can tell the user.

I saw this mostly as a way to streamline, standardise something with some reuse in a way we don't normally do and generally start introducing a little sanity into a 1.3MLOC codebase in a way people will get. Code only started throwing exceptions inside the last six months, in stuff I wrote. Exception handling is a concept I'm only just beginning to teach. You know, baby steps and all that.

dkf

@Arantor said in A little light (not entirely) WTF for a WTF codebase:

The same code could conceivably fail. I just don't care if it does.

As long as whenever the body fails, the overall program is still in a state where the assumptions of the outer world are correct, it's not a problem to just drop the exception. But you need to make sure that failures don't leave dangling resources or anything like that; maybe that's trivially easy, but you still gotta do it.

I tend to not leave empty catch blocks by putting in a TRACE-level logging action that I usually keep switched off. ;)

Kian

@Arantor While I understand doing hacks in the name of expedience, you still have to keep in mind that blindly ignoring exceptions isn't going to just ignore the exceptions you know about. It's going to ignore the exceptions you don't know about too. Maybe there's some error in the thumbnail code that only shows up under certain conditions. Maybe the bug is introduced a month from now. With this, you could end up with a file that should have a thumbnail not having it, and no warning that it failed. Some other part of the system may blow up because of that inconsistency. And good luck tracking down that kind of bug six months down the road when you forget all about this quick fix.

I'd strongly suggest either identifying exactly the kind of exception you want to ignore or checking before the call if possible. But, I know you're in a place of madness, and maybe adding chaos in the short term helps with clearing the chaos in the long term.

Arantor

@dkf correct, if this fails, all assumptions about outside world are left intact, because this only updates things strictly in the event it succeeded.

I'm not totally retarded ;)

dkf

@Arantor Never said you were, but I also write for the peanut gallery. ;)

blakeyrat

@Arantor I dunno what language this is, but if you're not going to use the $e variable, you might as well not tell the computer to create one for you.

blakeyrat

@Arantor Does the thumbnail process write to disk? What if the exception is, "disk space full"?

Vaire

@Arantor Yup, I've done that, and cared about as much as you did.

Frequently when I do something like that, it is because I am dealing with a KNOWN unreliable source or issue, that I have no control over, and sometimes the bloody thing is just going to fail. I setup a catch so it doesn't bring down the whole application, and move on with my life. I made the mistake of setting one of those to log and e-mail me whenever it was caught back in my early days. AND I was smart enough to do that on a FRIDAY. Yeaaaaah ... had to come into the office over the damn weekend to unplug THAT hot mess from pinging my e-mail so hard, it couldn't wear white to its wedding anymore o_O

Then I would just have it log. But they were logs i never looked at, or cared about. It was something out of my control, so why bother? Eventually I yanked the logging code as well and just put a comment in there of /*bugger it, this isn't my fault, go yell at ENTITY-X if you want this to go away :P */

Arantor

@blakeyrat this is PHP where the "compiler" isn't that smart.

Also, if we're out of disk space there are far, far larger problems than "writing thumbnails" to deal with, like our databases potentially corrupting because MySQL isn't especially clever about that.

blakeyrat

@Arantor said in A little light (not entirely) WTF for a WTF codebase:

Also, if we're out of disk space there are far, far larger problems than "writing thumbnails" to deal with, like our databases potentially corrupting because MySQL isn't especially clever about that.

Correct; which is exactly why you'd want to know that AS SOON AS POSSIBLE.

(That was kind of my point.)

Vaire

@Arantor said in A little light (not entirely) WTF for a WTF codebase:

@blakeyrat this is PHP where the "compiler" isn't that smart.

[...]because MySQL isn't especially clever about that.

I like the phrasing of that. "Isn't especially clever about that." Seems very British. Gonna have ta steal this, you know, for the cause. ;)

Arantor

@blakeyrat I forget you haven't seen the lounge thread about this where I explain how much of a mess the code base is.

If disk space actually ran out, it would fail much earlier in the process than my recent changes would, in fact pretty much everything would grind to a halt long before my code. We have a code base, even before you factor the PHP evil into it.