Simplify "_get_current_job" with exceptions

[okurz]

After

• #4574

We have cleaner and safer code if we can avoid the explicit value checking with exceptions.

Related progress issue: https://progress.opensuse.org/issues/108662

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 97.98%. Comparing base (580ca5d) to head (f071dd2). Report is 2116 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4575      +/-   ##
==========================================
- Coverage   97.98%   97.98%   -0.01%     
==========================================
  Files         375      375              
  Lines       34331    34327       -4     
==========================================
- Hits        33639    33635       -4     
  Misses        692      692              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[mergify[bot]]

This pull request is now in conflicts. Could you fix it? 🙏

[perlpunk]

I agree that exceptions should be used for such cases. But I wouldn't just use a simple die, but something like NotFoundException('message'). And to actually turn that into a 404 such exceptions must be catched right before Mojolicious renders the result. It would actually be nice if ->reply->not_found had an option to do that automaticaly for us.

[Martchus]

And to actually turn that into a 404 such exceptions must be catched right before Mojolicious renders the result.

Maybe that's possible via some hook. I suppose @kraih would know :-)

[kraih]

Maybe that's possible via some hook. I suppose @kraih would know :-)

You can always just replace the ->reply->exception helper to make whatever you want work. But i think i disagree with using exceptions for input validation. It's the "using exceptions for flow control" anti-pattern.

[okurz]

But i think i disagree with using exceptions for input validation. It's the "using exceptions for flow control" anti-pattern.

How can we prevent that we need to explicitly check for a null-value everywhere with the risk of forgetting it somewhere as happened for https://progress.opensuse.org/issues/108662

[kraih]

How can we prevent that we need to explicitly check for a null-value everywhere with the risk of forgetting it somewhere as happened for https://progress.opensuse.org/issues/108662

Skimming through the code, it seems that testid is a required route placeholder with number validation. Why do you need to do any null checks at all if it is already validated by the router? To me it seems like there is simply a bug somewhere that causes a route without testid placeholder to end up at a method that requires one.

[perlpunk]

Skimming through the code, it seems that testid is a required route placeholder with number validation. Why do you need to do any null checks at all if it is already validated by the router? To me it seems like there is simply a bug somewhere that causes a route without testid placeholder to end up at a method that requires one.

The problem is that testid is given, but the job with that id does not exist in the database.

That's why I commented here: https://github.com/os-autoinst/openQA/pull/4575#pullrequestreview-917044750

[kraih]

The problem is that testid is given, but the job with that id does not exist in the database.

I see, in that case you do have to check everywhere it can happen (and abstract out the repetitive parts). But throwing exceptions is really not a good solution.

[kraih]

But throwing exceptions is really not a good solution.

Perhaps i should elaborate a bit more. It's not just that using exceptions for flow control is already considered an anti-pattern. But the fact that many operations in Mojolicious can be async as well. Your exception catching mechanism to generate 4xx/5xx responses would only work contextual, when async features are not being used at all. As soon as an async feature sneaks in you would start getting connection timeouts, because the exception gets lost in the event loop and no response gets rendered at all (and there's a million variations with similar error conditions that will be pretty hard to track down...).

[Martchus]

It's not just that using exceptions for flow control is already considered an anti-pattern.

I'm not sure whether I'm following. Exceptions - regardless the cases in which they are used - alter the control flow. However, they're usually preserved to be used only in exceptional cases (such as error conditions). Not sure whether input validation counts as "exceptional" case, though. It is likely more the "daily business" that particular piece of code has to deal with.

As soon as an async feature sneaks in you would start getting connection timeouts, because the exception gets lost in the event loop and no response gets rendered at all

It is true that one has to handle an exception before the control flow returns to the event loop (because the event loop can normally not handle it gracefully anymore). But when handling exceptions before returning to the event loop I don't see a reason for not using them.

By the way, in another project using C++/Boost.Beast I'm throwing exceptions for bad requests and catch them before returning to the event loop. It works just fine and it achieves what @okurz has in mind - no extra handling on all the levels within the call stack. Of course in C++ one can easily catch specific exception classes and Boost.Beast is a thin HTTP library and not a framework imposing certain patterns/style. But here we are using the Mojolicious framework and I would avoid going against its designated usage patterns because "thinking outside the framework" is usually not the best approach (when using a framework).

[kraih]

It is true that one has to handle an exception before the control flow returns to the event loop (because the event loop can normally not handle it gracefully anymore). But when handling exceptions before returning to the event loop I don't see a reason for not using them.

You can do that, but the original idea was to hook into the existing exception handling mechanism of Mojolicious, which does not cover async use cases. So, you now need to develop a custom try/catch mechanism that knows how to handle your magical exceptions, and use it consistently all over the openQA code (or you just end up with even more paradigms every new developer needs to learn).

[Martchus]

The place in the C++ example where I'm catching the exception is in our case somewhere within Mojolicious, likely the function which calls the controller function. Judging by your answer that's not going to work with Mojolicious, though.

… the existing exception handling mechanism of Mojolicious, which does not cover async use cases.

Which actually makes sense. Also in my C++ example there's that limitation. So if one does not throw an exception but also does not provide a response immediately and instead the control flow goes back to the event loop which only at some point later invokes an additional handler to compute the response one cannot expect any exceptions from that additional handler to be catched and handled automatically anymore¹. (For the event loop the code is just an arbitrary handler with no association to a specific HTTP request so it cannot do anything about it.) It isn't a problem in my case because I take care to throw exceptions only in the immediate handler function of the route - but there it actually is very useful to be able to throw exceptions because almost all error conditions are already determined here. Of course this means that there are generally two ways of returning a 4xx error in my application and only one of the ways is always possible. I understand if you don't want to have two ways to do the same thing in Mojolicious.

So, you now need to develop a custom try/catch mechanism …

… which would be a try-catch on the level of controller functions I suppose. We do that already in certain controller functions in certain cases. However, I'm also against making it the rule as it seems more cumbersome in general.

---

¹ Except the exception would provide some way to do that, e.g. the callback to pass an error reponse to. But that kind of complexity is likely not worth it.

[okurz]

I don't know yet what is the best approach we could do here to avoid explicit null-value checking everywhere. I am ok with checking pre-conditions with simple die … unless … lines, following https://wiki.c2.com/?DesignByContract , but as you stated that would mean we would return a 5xx error and not a 4xx error to the user. So there is already some magic that ensures that there is a 5xx error fed back to users. So what do you recommend we can do?

[Martchus]

that there is a 5xx error fed back to users.

You mean a 4xx error.

So what do you recommend we can do?

There are multiple difficulties:

1. When catching an exception we needed to distinguish between different types of exceptions because unexpected exceptions should still result in a 5xx error and an error in the server log. In the C++ example I'm catching a specific BadRequest class. Unfortunately that's not possible in Perl so we needed at least some if-else in the catch- or if ($@)-bock which checks whether we've catched a blessed object of a special type.
2. We need to figure out where to add this catch-block. It should likely be the function that invokes the controller function so we don't have to add it in every controller function ourselves. However, that's then a function within the Mojolicious framework which complicates things.
3. Even if we figure out 1. and 2. we need to be aware that throwing exceptions and expecting them to be handled by responding to the corresponding HTTP request is not possible when the controller code returns the control flow back to the event loop at some point. E.g. when the response is rendered in the handler of an asynchronous operation we still must not throw an exception. So we will still have to render a 4xx response manually in these cases which means we end up with two ways of doing the same thing.

So considering these difficulties I'd say the best approach is to keep using return $self->render(status => 400, …) unless … in controller functions to keep it simple and in-line with how Mojolicious is intended to be used. If it means passing errors via return codes though multiple levels of function invocations then that's what we do. If it gets to annoying in certain cases we can still throw an exception in some nested function as long as we handle it in the controller function to render the 4xx response there. That's also what we already do sometimes.

[okurz]

that there is a 5xx error fed back to users.

You mean a 4xx error.

No, I meant, what is currently making sure that a user gets a 5xx error? That's from openQA, not just the apache proxy, right? So there is already a response when we put in a "die", right?

So what do you recommend we can do?

There are multiple difficulties:

1. When catching an exception we needed to distinguish between different types of exceptions because unexpected exceptions should still result in a 5xx error and an error in the server log. In the C++ example I'm catching a specific `BadRequest` class. Unfortunately that's not possible in Perl so we needed at least some if-else in the catch- or `if ($@)`-bock which checks whether we've catched a blessed object of a special type.

Why not a specific exception class and just a plain "die" to distinguish but still use exceptions?

[…] So considering these difficulties I'd say the best approach is to keep using return $self->render(status => 400, …) unless … in controller functions to keep it simple and in-line with how Mojolicious is intended to be used. If it means passing errors via return codes though multiple levels of function invocations then that's what we do. If it gets to annoying in certain cases we can still throw an exception in some nested function as long as we handle it in the controller function to render the 4xx response there. That's also what we already do sometimes.

ok, makes sense. Maybe I can do especially the "throw exception inside, catch and return response outside"-approach a bit more then.

[kraih]

Why not a specific exception class and just a plain "die" to distinguish but still use exceptions?

If you want to go in that direction, Mojolicious does contain some utility functions (check and raise) that might help.

[okurz]

discussed in our weekly estimation meeting 2022-03-31. If you have specific suggestions what to do in the code we could hack on something together, e.g. in a "mob programming session".

[mergify[bot]]

This pull request is now in conflicts. Could you fix it? 🙏

[mergify[bot]]

This pull request is now in conflicts. Could you fix it? 🙏