Using Try-Catch-Finally to process code for API endpoint - What are the drawbacks?

Question

I know this is bad design and I am trying to find any reason to not do this, so I need some help.

I have an API endpoint that is being hit and needs to respond back with an OK response within a few seconds at most. Currently we are taking the call and processing a lot of data and then returning the OK, however, if the DoLotsOfWork method times out, no OK response is being returned.

public IActionResult DoWork([FromBody] MyDTO myDto){
     DoLotsOfWork();
    return OK()
}

Now I've suggested that we migrate over to some sort of Queue and make the job run there but I am being told no as that work would take to long. Instead I am being told to do this.

public IActionResult DoWork([FromBody] MyDTO myDto){
   try{
       return OK();
   }
   catch{
   }
   finally{
      DoLotsOfWork();
   }  
}

Does anyone know of any drawbacks by doing things this way?

Answer 1

Since the actual question appears to be entirely "What are the drawbacks?", and you appear to know how the try-catch-finally is working as hacky a way to execute code after the API responds (despite some people in the comments and answers explaining what it does), I'll just link to this very well-written StackOverflow answer to a similar question authored by Stephen Cleary.

Edit 2: vatbub's answer below listing issues with that approach makes a good point in #4 (1-3 mostly reiterate try-catch misuse), and the description that follows elaborates.

Edit: In case it disappears from the internet:

And I need to "fire and forget"

I have a blog post that goes into details of several different approaches for fire-and-forget on ASP.NET.

In summary: first, try not to do fire-and-forget at all. It's almost always a bad idea. Do you really want to "forget"? As in, not care whether it completes successfully or not? Ignore any errors? Accept occasional "lost work" without any log notifications? Almost always, the answer is no, fire-and-forget is not the appropriate approach.

A reliable solution is to build a proper distributed architecture. That is, construct a message that represents the work to be done and queue that message to a reliable queue (e.g., Azure Queue, MSMQ, etc). Then have an independent backend that process that queue (e.g., Azure WebJob, Win32 service, etc).

Should I just call Task.Run() without any async-await?

No. This is the worst possible solution. If you must do fire-and-forget, and you're not willing to build a distributed architecture, then consider Hangfire. If that doesn't work for you, then at the very least you should register your cowboy background work with the ASP.NET runtime via HostingEnvironment.QueueBackgroundWorkItem or my ASP.NET Background Tasks library. Note that QBWI and AspNetBackgroundTasks are both unreliable solutions; they just minimize the chance that you'll lose work, not prevent it.

Answer 2

There's two problems with your approach:

1. That would only work with end points that don't return anything. How can you return something before you build the object to return?

2. The try block is, presumably, to "handle" (spoiler alert, you're not handling anything) exceptions in your code, right? Well, it's not the Ok() call that can throw, it's your code, the one in finally. Unless you also put that in a try, any exceptions in it will still be unhandled, thus defeating the only possible reason I see to write code like that in the first place.

Answer 3

Okay, so the major problem is:

You are going to tell whomever sent the API call, that the response was okay. And if "DoLotsOfWork" times out, or throws any kind of exception, that little nugget of boom, will actually force a restart of the API or shut it down, depending on how it is deployed.

Answer 4

Your design is flawed for multiple reasons, from least to most important:

1. This is not the intended use for try-catch-finally and fellow developers in your team will look at this and will have trouble understanding what's going on.
2. Bad API design: As pointed out by other answers, you are lying to the client that everything is OK, even though you have no idea yet if that's the case.
3. Missing error handling: If DoLotsOfWork()throws an exception, you will either not get notified or worse, your entire server process may crash, because the exception is not caught.
4. Most importantly, your thread will still block until DoLotsOfWork() is done, making your application susceptible for denial-of-service attacks.

To solve your issues, you need to change your API design. By returning OK, you are obviously lying to the client. In a perfect world, you would have an API-endpoint where the client can submit the data to process, and you would return something along the lines of "Your data was successfully scheduled for processing". You would then have another endpoint where the client could check the progress of the processing.

Changing your API design in this way, however, does take a lot of time, which is probably why your boss is hesitant to do this.

However, if you neglect the fact that you are lying to your client, and still return OK and offer no additional API endpoint for progress checking, you can still either have a separate processing thread that queues up all the data or just spawn a new thread in place for each processing job. The effort to implement such a solution is pretty much the same as for implementing the hacky try-catch-finally solution, but it still saves you from the possibility of being DDOSed.