Optionally return HTTP_OK for RPC errors #15381
Conversation
|
NAK the flag, I don't think there should be a flag to tolerate bad client libraries. I think it's one of those flags that can be easily left enabled just because.. I don't know |
|
I'm also skeptical about this, doesn't seem like something that should be solved at the server side. It complicates our code even more. I mean, I could have understood this 5 years ago but our RPC implementation has been around so long that for every language there's something to interface with Certainly from python. Check our own functional tests for an example. |
|
Agree that we shouldn't need to workaround JSON library bugs. |
|
I also agree Bitcoin Core should not have ugly hacks just because of buggy JSON-RPC libraries. |
|
I understand that we shouldn't work around bad libraries (and I have no strong need for that in my own application), but I wonder if especially the 500 error codes are really correct to return. In RFC 2616, it says that 5xx codes are meant for "Server Error - The server failed to fulfill an apparently valid request". To me, that sounds like it certainly does not apply for things like invalid requests sent from the client (because then the request is not apparently valid). In those cases, we should return at least a 4xx error code (client error). But even there I find it a bit confusing that we are mixing errors on the higher-level JSON-RPC protocol (which has its own error reporting) with errors on the HTTP transport layer. For me, it would make more sense to return a 404 error only if, say, the RPC HTTP endpoint is not valid, rather than if the HTTP request was fine but it failed on the JSON-RPC side. The JSON-RPC 2.0 spec doesn't say anything about HTTP. There is a document mentioning error codes as they are used in Bitcoin at the moment, but it is not clear to me how authoritative it is (listed as historical). Is there any proper specification for how HTTP status codes should look like with JSON-RPC? |
This adds a new boolean option -rpcerrorhttpok (off by default). If it is turned on, then the HTTP server returns HTTP_OK (code 200) also for errors with JSON-RPC calls. The rationale is this: Some JSON-RPC libraries (e.g. Python jsonrpclib) do not expose the response body at all if HTTP already sets an error code. In that situation, it is hard to properly handle or debug the actual underlying error. In such a situation, the new option can be used to bypass this problem.
526bf2f to
42c6d6c
Compare
|
Independent of the actual change in HTTP response codes: Do you think just the new test code for the returned status codes is useful, e.g. to split out into a separate PR (just for the status quo without the new flag)? |
IIUC HTTP is only the transport, so you only apply HTTP rules there. For instance, an invalid JSON-RPC request, like passing a wrong argument type, should not be 500 or 400, but 200 with the JSON-RPC error -32600. |
That's exactly what my intuitive opinion (without being an expert) also is - that's what I tried to express with my post above. Although, of course, even if that's correct, then we may still not want to simply change the behaviour after a couple years. |
|
@domob1812 maybe the implementation followed https://www.jsonrpc.org/historical/json-rpc-over-http.html |
Yes, I saw that (and linked to the doc in my post). I'm not sure how relevant / authoritative that document is, since it is linked as "historical" and not really states what formal status (if any) it has. |
This is kind of my point. Even though some things about the RPC interface are kooky in some sense, over the last 10 years, many have adopted libraries to specifically how it works (thus far that In the long term is might make sense to do an overhaul to "normalize" the implementation to the JSON RPC 2.0 standard (if things are really mentioned in the standard). But this is different from "change a few minor things to satisfy a specific implementation". I also remember that has been tried before (see e.g. #12435) but it attracts very little attention; pedantics aside, it works as-is and people are unwilling to change things. Changing it now results in extra work—everything interfacing to |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
Return HTTP_OK (code 200) also for JSON-RPC calls that fail on the RPC side. This makes sense, since those have a separate error code anyway through JSON-RPC itself. HTTP error codes should signify actual errors on the HTTP transport side. Some JSON-RPC libraries make it hard or impossible to properly extract the JSON-RPC error code if HTTP returns non-200. This change makes life easier with those (and makes everything in general more consistent). See also the discussion for upstream Bitcoin: bitcoin/bitcoin#15381
Return HTTP_OK (code 200) also for JSON-RPC calls that fail on the RPC side. This makes sense, since those have a separate error code anyway through JSON-RPC itself. HTTP error codes should signify actual errors on the HTTP transport side. Some JSON-RPC libraries make it hard or impossible to properly extract the JSON-RPC error code if HTTP returns non-200. This change makes life easier with those (and makes everything in general more consistent). See also the discussion for upstream Bitcoin: bitcoin/bitcoin#15381
Return HTTP_OK (code 200) also for JSON-RPC calls that fail on the RPC side. This makes sense, since those have a separate error code anyway through JSON-RPC itself. HTTP error codes should signify actual errors on the HTTP transport side. Some JSON-RPC libraries make it hard or impossible to properly extract the JSON-RPC error code if HTTP returns non-200. This change makes life easier with those (and makes everything in general more consistent). See also the discussion for upstream Bitcoin: bitcoin/bitcoin#15381
Return HTTP_OK (code 200) also for JSON-RPC calls that fail on the RPC side. This makes sense, since those have a separate error code anyway through JSON-RPC itself. HTTP error codes should signify actual errors on the HTTP transport side. Some JSON-RPC libraries make it hard or impossible to properly extract the JSON-RPC error code if HTTP returns non-200. This change makes life easier with those (and makes everything in general more consistent). See also the discussion for upstream Bitcoin: bitcoin/bitcoin#15381
Return HTTP_OK (code 200) also for JSON-RPC calls that fail on the RPC side. This makes sense, since those have a separate error code anyway through JSON-RPC itself. HTTP error codes should signify actual errors on the HTTP transport side. Some JSON-RPC libraries make it hard or impossible to properly extract the JSON-RPC error code if HTTP returns non-200. This change makes life easier with those (and makes everything in general more consistent). See also the discussion for upstream Bitcoin: bitcoin/bitcoin#15381
Return HTTP_OK (code 200) also for JSON-RPC calls that fail on the RPC side. This makes sense, since those have a separate error code anyway through JSON-RPC itself. HTTP error codes should signify actual errors on the HTTP transport side. Some JSON-RPC libraries make it hard or impossible to properly extract the JSON-RPC error code if HTTP returns non-200. This change makes life easier with those (and makes everything in general more consistent). See also the discussion for upstream Bitcoin: bitcoin/bitcoin#15381
8f5d943 Add regtests for HTTP status codes. (Daniel Kraft) Pull request description: This adds explicit tests for the returned HTTP status codes to `interface_rpc.py` (for error cases) and the HTTP JSON-RPC client in general for success. #15381 brought up discussion about the HTTP status codes in general, and the general opinion was that the current choice may not be ideal but should not be changed to preserve compatibility with existing JSON-RPC clients. Thus it makes sense to actually test the current status to ensure this desired compatibility is not broken accidentally. ACKs for commit 8f5d94: laanwj: utACK 8f5d943 promag: utACK 8f5d943. jonasschnelli: utACK 8f5d943 Tree-SHA512: 82503ccd134dd9145304e95cb6c61755f100bee27593d567cdd5c0c554d47e7b06d937456cab04107f46f4984930355db65d5e711008a0b05f2b8feec9f2950e
|
With #15495 merged, we can close this now. |
Return HTTP_OK (code 200) also for JSON-RPC calls that fail on the RPC side. This makes sense, since those have a separate error code anyway through JSON-RPC itself. HTTP error codes should signify actual errors on the HTTP transport side. Some JSON-RPC libraries make it hard or impossible to properly extract the JSON-RPC error code if HTTP returns non-200. This change makes life easier with those (and makes everything in general more consistent). See also the discussion for upstream Bitcoin: bitcoin/bitcoin#15381
b5c3195 Return HTTP_OK for RPC errors (Daniel Kraft) Pull request description: Return `HTTP_OK` (code 200) also for JSON-RPC calls that fail on the RPC side. This makes sense, since those have a separate error code anyway through JSON-RPC itself. HTTP error codes should signify actual errors on the HTTP transport side. Some JSON-RPC libraries make it hard or impossible to properly extract the JSON-RPC error code if HTTP returns non-200. This change makes life easier with those (and makes everything in general more consistent). See also the [discussion for upstream Bitcoin](bitcoin/bitcoin#15381). ACKs for commit b5c319: Tree-SHA512: 1190aba2a1eda1bf58fd45dc1e4495ec0722d8768fc36e64bae026d738fcf23e6fa7f590389eaa87460e1307c804f49636ed1263c28e43b3020fafb07ff0f768
Summary: PR description: > This adds explicit tests for the returned HTTP status codes to > interface_rpc.py (for error cases) and the HTTP JSON-RPC client in > general for success. > > [[bitcoin/bitcoin#15381 | PR15381]] brought up discussion about the HTTP status codes in general, > and the general opinion was that the current choice may not be ideal > but should not be changed to preserve compatibility with existing > JSON-RPC clients. Thus it makes sense to actually test the current > status to ensure this desired compatibility is not broken accidentally. This is a backport of Core [[bitcoin/bitcoin#15495 | PR15495]] Test Plan: `ninja && test/functional/test_runner.py interface_rpc` Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D7752
8f5d943 Add regtests for HTTP status codes. (Daniel Kraft) Pull request description: This adds explicit tests for the returned HTTP status codes to `interface_rpc.py` (for error cases) and the HTTP JSON-RPC client in general for success. bitcoin#15381 brought up discussion about the HTTP status codes in general, and the general opinion was that the current choice may not be ideal but should not be changed to preserve compatibility with existing JSON-RPC clients. Thus it makes sense to actually test the current status to ensure this desired compatibility is not broken accidentally. ACKs for commit 8f5d94: laanwj: utACK 8f5d943 promag: utACK 8f5d943. jonasschnelli: utACK 8f5d943 Tree-SHA512: 82503ccd134dd9145304e95cb6c61755f100bee27593d567cdd5c0c554d47e7b06d937456cab04107f46f4984930355db65d5e711008a0b05f2b8feec9f2950e
8f5d943 Add regtests for HTTP status codes. (Daniel Kraft) Pull request description: This adds explicit tests for the returned HTTP status codes to `interface_rpc.py` (for error cases) and the HTTP JSON-RPC client in general for success. bitcoin#15381 brought up discussion about the HTTP status codes in general, and the general opinion was that the current choice may not be ideal but should not be changed to preserve compatibility with existing JSON-RPC clients. Thus it makes sense to actually test the current status to ensure this desired compatibility is not broken accidentally. ACKs for commit 8f5d94: laanwj: utACK 8f5d943 promag: utACK 8f5d943. jonasschnelli: utACK 8f5d943 Tree-SHA512: 82503ccd134dd9145304e95cb6c61755f100bee27593d567cdd5c0c554d47e7b06d937456cab04107f46f4984930355db65d5e711008a0b05f2b8feec9f2950e
8f5d943 Add regtests for HTTP status codes. (Daniel Kraft) Pull request description: This adds explicit tests for the returned HTTP status codes to `interface_rpc.py` (for error cases) and the HTTP JSON-RPC client in general for success. bitcoin#15381 brought up discussion about the HTTP status codes in general, and the general opinion was that the current choice may not be ideal but should not be changed to preserve compatibility with existing JSON-RPC clients. Thus it makes sense to actually test the current status to ensure this desired compatibility is not broken accidentally. ACKs for commit 8f5d94: laanwj: utACK 8f5d943 promag: utACK 8f5d943. jonasschnelli: utACK 8f5d943 Tree-SHA512: 82503ccd134dd9145304e95cb6c61755f100bee27593d567cdd5c0c554d47e7b06d937456cab04107f46f4984930355db65d5e711008a0b05f2b8feec9f2950e
This adds a new boolean option
-rpcerrorhttpok(off by default). If it is turned on, then the HTTP server returnsHTTP_OK(code 200) also for errors with JSON-RPC calls.The rationale is this: Some JSON-RPC libraries (e.g. Python
jsonrpclib) do not expose the response body at all if HTTP already sets an error code. In that situation, it is hard to properly handle or debug the actual underlying error. In such a situation, the new option can be used to bypass this problem.Also includes general regtests for the returned HTTP status codes (not just in the new situation but also for the existing logic).