Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deal with broken deployments #22

Closed
4 tasks done
edsko opened this issue Jun 28, 2023 · 4 comments
Closed
4 tasks done

Deal with broken deployments #22

edsko opened this issue Jun 28, 2023 · 4 comments
Assignees
Labels
enhancement New feature or request priority: medium Should be done before the library can be considered complete spec Part of the spec is not implemented (use bug or enhancement label to distinguish further)

Comments

@edsko
Copy link
Collaborator

edsko commented Jun 28, 2023

The spec says: Implementations should expect broken deployments to

Implementations must synthesize a Status & Status-Message to propagate to the application layer when this occurs.

Note that the spec specifically states this about dealing with responses (i.e., client-side). Indeed, for dealing with requests (server-side) the spec has different requirements; for example, a missing content-type header should result in a HTTP 415 status.

Additionally:

At worst, the implementation can abort decoding the status message altogether such that the user would received the raw percent-encoded form. Alternatively, the implementation can decode valid portions while leaving broken %-encodings as-is or replacing them with a replacement character (e.g., '?' or the Unicode replacement character).

We don't do this yet.

@edsko edsko added the bug Something isn't working label Jun 28, 2023
@edsko edsko added enhancement New feature or request spec Part of the spec is not implemented (use bug or enhancement label to distinguish further) priority: medium Should be done before the library can be considered complete and removed bug Something isn't working labels Jan 20, 2024
@domenkozar
Copy link

I get the following back:

 SomeExceptionWrapper (STMException [("atomically",SrcLoc {srcLocPackage = "grapesy-0.1.0-KFclJYWAZ3d94upf4VowEP", srcLocModule = "Network.GRPC.Util.Thread", srcLocFile = "src/Network/GRPC/Util/Thread.hs", srcLocStartLine = 203, srcLocStartCol = 7, srcLocEndLine = 203, srcLocEndCol = 17}),("withThreadInterface",SrcLoc {srcLocPackage = "grapesy-0.1.0-KFclJYWAZ3d94upf4VowEP", srcLocModule = "Network.GRPC.Util.Session.Channel", srcLocFile = "src/Network/GRPC/Util/Session/Channel.hs", srcLocStartLine = 339, srcLocStartCol = 5, srcLocEndLine = 339, srcLocEndCol = 24}),("recv",SrcLoc {srcLocPackage = "grapesy-0.1.0-KFclJYWAZ3d94upf4VowEP", srcLocModule = "Network.GRPC.Client.Call", srcLocFile = "src/Network/GRPC/Client/Call.hs", srcLocStartLine = 131, srcLocStartCol = 32, srcLocEndLine = 131, srcLocEndCol = 44})] (SomeExceptionWrapper (ThreadInterfaceUnavailable {threadInterfaceCallStack = [("withThreadInterface",SrcLoc {srcLocPackage = "grapesy-0.1.0-KFclJYWAZ3d94upf4VowEP", srcLocModule = "Network.GRPC.Util.Session.Channel", srcLocFile = "src/Network/GRPC/Util/Session/Channel.hs", srcLocStartLine = 339, srcLocStartCol = 5, srcLocEndLine = 339, srcLocEndCol = 24}),("recv",SrcLoc {srcLocPackage = "grapesy-0.1.0-KFclJYWAZ3d94upf4VowEP", srcLocModule = "Network.GRPC.Client.Call", srcLocFile = "src/Network/GRPC/Client/Call.hs", srcLocStartLine = 131, srcLocStartCol = 32, srcLocEndLine = 131, srcLocEndCol = 44})], threadInterfaceException = ResponseInvalidStatus (Status {statusCode = 404, statusMessage = "Not Found"})})))

It would be great if the response and request were included to be able to understand what's happening.

@edsko
Copy link
Collaborator Author

edsko commented Feb 13, 2024

This issue is not about connectivity issues, but about non-compliant gRPC implementations that use HTTP error codes instead of gRPC error codes to indicate problems (gRPC problems are meant to be indicated with a HTTP error of 200 and then a non-zero grpc-status). The 404/not found in your message suggests it's not able to connect to the remote server at all? Not sure what kind of request/response you're hoping to see; in particular, request, response in gRPC terms (the messages sent to and from the server) have not yet happened at the connecting stage.

edsko added a commit that referenced this issue Feb 17, 2024
In the process, this makes a few other improvements:

* The client can now insist on an initial compression algorithm,
  before compression negotation has taken place (`connInitCompression`)
* The server can insist on a particular compression algorithm,
  independent of what the client offers (`insist`)
* When the server returns a HTTP status other than 200, we include the
  response body in the exception (this might help with #22, specifically
  #22 (comment))
@edsko edsko changed the title Deal with HTTP status other than 200 Deal with broken deployments Jul 18, 2024
@edsko edsko self-assigned this Jul 23, 2024
@edsko
Copy link
Collaborator Author

edsko commented Jul 26, 2024

In response to @domenkozar 's comment above: as I mentioned previously, grapesy now includes the full response body in case of server errors. As of #197, a 404 Not Found from the server will now result in a GrpcUnimplemented exception; I have added a test in #207 to ensure that we still include the response body in this gRPC exception.

@edsko
Copy link
Collaborator Author

edsko commented Jul 26, 2024

All aspects of this ticket I think are now working and tested. Also verified that the few changes we made did not break the interop tests. Closing!

@edsko edsko closed this as completed Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority: medium Should be done before the library can be considered complete spec Part of the spec is not implemented (use bug or enhancement label to distinguish further)
Projects
None yet
Development

No branches or pull requests

2 participants