[release-1.12] Fix handling errors in HTTP service invocation #7327
Conversation
1. Make sure error is always logged 2. If data has already been sent to users, log the error only and do not append it to the response, which could cause data corruption for the caller Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com>
|
@ItalyPaleAle please can we include tests so we don't have a regression/covers the bug introduced in the previous PR |
ItalyPaleAle
changed the title
Not a regression introduced by the other PR, just something else that was left out: adding some logs and making sure errors aren't appended to the body (something observed with 1.12.0 too). Not sure we can create a test for this as it requires creating complex situations such as gRPC connections failing while a request is in progress. I've been working with @yaron2 and 2 customers this morning and found that this change helped identifying a bug with the gRPC stream between sidecars. We are still investigating the root cause of that bug. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## release-1.12 #7327 +/- ##
================================================
- Coverage 64.81% 64.76% -0.06%
================================================
Files 231 231
Lines 20940 20955 +15
================================================
- Hits 13573 13571 -2
- Misses 6234 6251 +17
Partials 1133 1133 ☔ View full report in Codecov by Sentry. |
In the case of code being untestable we should refactor the code to enable introducing mid request errors via a middleware, though it looks like this should be possible with the code today AFAICT. |
| // Use Warn log here because it's the only way users are notified of the error | ||
| log.Warnf("HTTP service invocation failed to complete with error: %v", err) | ||
|
|
||
| // Do nothing else, as at least some data was already sent to the client |
There was a problem hiding this comment.
Does it mean the user will get a successful response with incomplete data?
There was a problem hiding this comment.
No, if the response was successful, the data is already sent to the client within the policy function.
If we are here, it means that the request failed. There are 2 possible options here:
- The request failed without ever sending any data to the user. In this case, down below we send a response to the client with a HTTP status code indicating an error, and the error details in the body
- The request failed but at least a partial response has already been sent to the client. This is what we've observed when working on Fix timeouts in HTTP service invocation when resiliency policies with timeouts are applied #7270 (which was a bug in Dapr now fixed) and now in bugs reported by 2 separate users (which seem to be due to the gRPC connection between sidecars failing - we are investigating that separately). In this case, we were here trying to re-set the status code (pointlessly since the headers have already been flushed), and more importantly we were appending the error message to the response body, which meant we were sending a "corrupted" response to the caller.
The main change here is recognizing that if some data has already been sent to the client, we should abort the request without sending more data.
At the same time, we were never logging errors. Now errors are logged with debug level if they are also sent to the client in the body; if not, they are logged as warnings.
Cherry-pick #7324 into release-1.12