如何修复“本地捕获的‘抛出’异常”?

在这个处理 REST API 调用的函数中,任何被调用来处理部分请求的函数都可能抛出一个错误,以表明应该将错误代码作为响应发送。但是,函数本身也可能发现错误,此时它应该跳转到异常处理块。

static async handleRequest(req) {
try {
let isAllowed = await checkIfIsAllowed(req);
if (!isAllowed) {
throw new ForbiddenException("You're not allowed to do that.");
}
let result = await doSomething(req); // can also raise exceptions
sendResult(result);
} catch(err) {
sendErrorCode(err);
}
}

Webstorm 将用以下信息在 throw下面划线: 'throw' of exception caught locally. This inspection reports any instances of JavaScript throw statements whose exceptions are always caught by containing try statements. Using throw statements as a "goto" to change the local flow of control is likely to be confusing.

但是,我不确定如何重构代码来改善这种情况。

我可以复制从 catch块到 if检查的代码,但我相信这会使我的代码更难读和维护。

我可以编写一个新函数来检查 isAllowed,如果不成功就抛出一个异常,但这似乎回避了这个问题,而不是修复 Webstorm 应该报告的设计问题。

我们使用异常的方式很糟糕吗? 这就是我们遇到这个问题的原因,还是说 Webstorm 错误只是误导了我们,应该禁用它?

51752 次浏览

You're checking for something and throwing an exception if isAllowed fails, but you know what to do in that situation - call sendErrorCode. You should throw exceptions to external callers if you don't know how to handle the situation - ie in exceptional circumstances.

In this case you already have a defined process of what to do if this happens - just use it directly without the indirect throw/catch:

static async handleRequest(req) {
try {
let isAllowed = await checkIfIsAllowed(req);
if (!isAllowed) {
sendErrorCode("You're not allowed to do that.");
return;
}
let result = await doSomething(req); // can also raise exceptions
sendResult(result);
} catch(err) {
sendErrorCode(err);
}
}

I could copypaste the code from the catch block into the ifcheck, but I believe this would make my code less readable and harder to maintain.

On the contrary, as above, I would expect this to be the way to handle this situation.

This could give you some tips, maybe that can be the cause(not sure if relevant). Catch statement does not catch thrown error

" The reason why your try catch block is failing is because an ajax request is asynchronous. The try catch block will execute before the Ajax call and send the request itself, but the error is thrown when the result is returned, AT A LATER POINT IN TIME.

When the try catch block is executed, there is no error. When the error is thrown, there is no try catch. If you need try catch for ajax requests, always put ajax try catch blocks inside the success callback, NEVER outside of it."

Answer of James Thorpe has one disadvantage on my opinion. It's not DRY, in both cases when you call sendError you handle Exceptions. Let's imagine we have many lines of code with logic like this where Exception can be thrown. I think it can be better.

This is my solution

async function doSomethingOnAllowedRequest(req) {
let isAllowed = await checkIfIsAllowed(req);
if (!isAllowed) {
throw new ForbiddenException("You're not allowed to do that.");
}
doSomething(req);
}
static async handleRequest(req) {
try {
let result = await doSomethingOnAllowedRequest(req);
sendResult(result);
} catch(err) {
sendErrorCode(err);
}
}

There are good answers to the question "Why not use exceptions as normal flow control?" here.

The reason not to throw an exception that you will catch locally is that you locally know how to handle that situation, so it is, by definition, not exceptional.

@James Thorpe's answer looks good to me, but @matchish feels it violates DRY. I say that in general, it does not. DRY, which stands for Don't Repeat Yourself, is defined by the people who coined the phrase as "Every piece of knowledge must have a single, unambiguous, authoritative representation within a system". As applied to writing software code, it is about not repeating complex code.

Practically any code that is said to violate DRY is said to be "fixed" by extracting the repeated code into a function and then calling that function from the places it was previously repeated. Having multiple parts of your code call sendErrorCode is the solution to fixing a DRY problem. All of the knowledge of what to do with the error is in one definitive place, namely the sendErrorCode function.

I would modify @James Thorpe's answer slightly, but it is more of a quibble than a real criticism, which is that sendErrorCode should be receiving exception objects or strings but not both:

static async handleRequest(req) {
try {
let isAllowed = await checkIfIsAllowed(req);
if (!isAllowed) {
sendErrorCode(new ForbiddenException("You're not allowed to do that."));
return;
}
let result = await doSomething(req); // can also raise exceptions
sendResult(result);
} catch(err) {
sendErrorCode(err);
}
}

The larger question is what is the likelihood of the error and is it appropriate to treat !isAllowed as an exception. Exceptions are meant to handle unusual or unpredictable situations. I would expect !isAllowed to be a normal occurrence that should be handled with logic specific to that situation, unlike, say, a sudden inability to query the database that has the answer to the isAllowed question.

@matchish's proposed solution changes the contract of doSomethingOnAllowedRequest from something that will never throw an exception to something that will routinely throw an exception, placing the burden of exception handling on all of its callers. This is likely to cause a violation of DRY by causing multiple callers to have repetitions of the same error handling code, so in the abstract I do not like it. In practice, it would depend on the overall situation, such as how many callers are there and do they, in fact, share the same response to errors.

Since this is not a blocking error, but only an IDE recommendation, then the question should be viewed from two sides.

The first side is performance. If this is a bottleneck and it is potentially possible to use it with compilation or when transferring to new (not yet released) versions of nodejs, the presence of repetitions is not always a bad solution. It seems that the IDE hints precisely in this case and that such a design can lead to poor optimization in some cases.

The second side is the code design. If it will make the code more readable and simplify the work for other developers - keep it. From this point of view, solutions have already been proposed above.

Contrary to James Thorpe's opinion, I slightly prefer the pattern of throwing. I don't see any compelling reason to treat local errors in the try block any differently from errors that bubble up from deeper in the call stack... just throw them. In my opinion, this is a better application of consistency.

Because this pattern is more consistent, it naturally lends itself better to refactoring when you want to extract logic in the try block to another function that is perhaps in another module/file.

// main.js
try {
if (!data) throw Error('missing data')
} catch (error) {
handleError(error)
}


// Refactor...


// validate.js
function checkData(data) {
if (!data) throw Error('missing data')
}


// main.js
try {
checkData(data)
} catch (error) {
handleError(error)
}

If instead of throwing in the try block you handle the error, then the logic has to change if you refactor it outside of the try block.

In addition, handling the error has the drawback of making you remember to return early so that the try block doesn't continue to execute logic after the error is encountered. This can be quite easy to forget.

try {
if (!data) {
handleError(error)
return // if you forget this, you might execute code you didn't mean to. this isn't a problem with throw.
}
// more logic down here
} catch (error) {
handleError(error)
}

If you're concerned about which method is more performant, you shouldn't be. Handling the error is technically more performant, but the difference between the two is absolutely trivial.

Consider the possibility that WebStorm is a bit too opinionated here. ESLint doesn't even have a rule for this. Either pattern is completely valid.

Return a promise reject instead of throwing error in the try block

  try {
const isAllowed = await checkIfIsAllowed(request);


if (!isAllowed) {
return Promise.reject(Error("You're not allowed to do that."));
}


const result = await doSomething(request);


sendResult(result);
} catch (error) {
throw error;
}