fix(destinations): Stop writing resources when channel is closed #460
Conversation
⏱️ Benchmark results
|
There was a problem hiding this comment.
Had one question given I didn't understand the fix.
Also, I don't know if possible but given that those affect all plugin can we add tests to that (e2e in memory tests available here https://github.com/cloudquery/plugin-sdk/tree/main/serve) ?
| @@ -264,6 +264,10 @@ func (c *DestinationClient) Write(ctx context.Context, source string, syncTime t | |||
| Source: source, | |||
| Timestamp: timestamppb.New(syncTime), | |||
| }); err != nil { | |||
| if err == io.EOF { | |||
There was a problem hiding this comment.
Im not sure i understand what this solved. give in othercase this just returns an error and doesn't write to the channel while with break it actually sends CloseAndRecv.
There was a problem hiding this comment.
Write works in stream mode (it's not a synchronous RPC), so if we want the correct response/error from the write request we need to call RecvMsg which is only done in CloseAndRecv (and only once actually).
So SendMsg will never return application level error as it doesn't wait for the message to be received by the server.
Technically I think we should call RecvMsg for each SendMsg per the docs:
> SendMsg does not wait until the message is received by the server. An untimely stream closure may result in lost messages. To ensure delivery, users should ensure the RPC completed successfully using RecvMsg.
We don't need ⬆️ as we don't have Bidi-streaming (I think), but still the response is received from CloseAndRecv, see https://grpc.io/docs/languages/go/generated-code/#client-streaming-methods-1
Sure I'll see how we can add a test for this |
|
@yevgenypats added a test in 03ab5d9. Without the fix the error will just be Once #461 is merged the error should change from |
|
Still happens and reported on Discord https://discord.com/channels/872925471417962546/873606591335759872/1057191097648033833 |
🤖 I have created a release *beep* *boop* --- ## [1.14.0](v1.13.1...v1.14.0) (2022-12-27) ### Features * Add basic periodic metric INFO logger ([#496](#496)) ([8d1d32e](8d1d32e)) ### Bug Fixes * **destinations:** Stop writing resources when channel is closed ([#460](#460)) ([5590845](5590845)) * Don't hide errors in destination server ([#529](#529)) ([d91f94f](d91f94f)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Summary
Related to cloudquery/cloudquery#5152.
This fixes an issue when if the channel gets closed (for example there's an error during write), we report the
EOFerror instead of the original error.Scenario from the issue:
sync --no-migrateon a non initialized databaseWritefails on the first table write, returning an error from this function:plugin-sdk/internal/servers/destinations.go
Line 65 in ab7ca97
for resource := range resourcesstarts, receiving anEOFerror since the channel is closed, andEOFerror is reported.Also, another related fix coming
Use the following steps to ensure your PR is ready to be reviewed
go fmtto format your code 🖊golangci-lint run🚨 (install golangci-lint here)