Optionally return HTTP_OK for RPC errors #15381

pull domob1812 wants to merge 1 commits into bitcoin:master from domob1812:httprpc-error-code changing 4 files +40 −11
  1. domob1812 commented at 12:04 pm on February 11, 2019: contributor

    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.

    Also includes general regtests for the returned HTTP status codes (not just in the new situation but also for the existing logic).

  2. fanquake added the label RPC/REST/ZMQ on Feb 11, 2019
  3. promag commented at 6:58 pm on February 11, 2019: member

    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 jsonrpclib but after a quick google search you can find a workaround.

  4. laanwj commented at 7:37 pm on February 11, 2019: member

    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 bitcoind.

    Certainly from python. Check our own functional tests for an example.

  5. in test/functional/test_framework/authproxy.py:147 in 526bf2f2c3 outdated 
    146 
147     def batch(self, rpc_call_list):
148         postdata = json.dumps(list(rpc_call_list), default=EncodeDecimal, ensure_ascii=self.ensure_ascii)
149         log.debug("--> " + postdata)
150-        return self._request('POST', self.__url.path, postdata.encode('utf-8'))
151+        response, status = self._request('POST', self.__url.path, postdata.encode('utf-8'))
    practicalswift commented at 8:33 pm on February 11, 2019:
    status is unused. If that is intentional then use the more idiomatic form: response, _ = ….
    domob1812 commented at 8:29 am on February 13, 2019:
    Fixed
  6. luke-jr commented at 6:42 pm on February 12, 2019: member
    Agree that we shouldn’t need to workaround JSON library bugs.
  7. kristapsk commented at 6:48 pm on February 12, 2019: contributor
    I also agree Bitcoin Core should not have ugly hacks just because of buggy JSON-RPC libraries.
  8. domob1812 commented at 8:28 am on February 13, 2019: contributor

    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?

  9. Optionally return HTTP_OK for RPC errors 
    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.
    42c6d6c8f4
  10. domob1812 force-pushed on Feb 13, 2019
  11. domob1812 commented at 8:30 am on February 13, 2019: contributor
    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)?
  12. promag commented at 3:20 pm on February 13, 2019: member

    The JSON-RPC 2.0 spec doesn’t say anything about HTTP

    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.

  13. domob1812 commented at 5:58 pm on February 13, 2019: contributor

    The JSON-RPC 2.0 spec doesn’t say anything about HTTP

    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.

  14. promag commented at 8:11 pm on February 13, 2019: member
    @domob1812 maybe the implementation followed https://www.jsonrpc.org/historical/json-rpc-over-http.html
  15. domob1812 commented at 7:11 am on February 14, 2019: contributor

    @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.

  16. laanwj commented at 8:15 am on February 15, 2019: member

    Although, of course, even if that’s correct, then we may still not want to simply change the behaviour after a couple years.

    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 btcd has a bitcoind compatibility mode!)

    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 bitcoind has to be changed!—and sure, it could be an option, but maintaining two different ways of doing things does increase maintenance burden.

  17. DrahtBot commented at 9:51 pm on February 26, 2019: member

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #15495 (Add regtests for HTTP status codes by domob1812)
    • #15483 (rpc: Adding a ’logpath’ entry to getrpcinfo by darosior)

    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.

  18. domob1812 referenced this in commit e52b672778 on Feb 27, 2019
  19. domob1812 referenced this in commit 15801eecf1 on Feb 27, 2019
  20. domob1812 referenced this in commit f9ebd82769 on Feb 27, 2019
  21. domob1812 referenced this in commit 9df1978513 on Feb 27, 2019
  22. domob1812 referenced this in commit 637b42250f on Mar 5, 2019
  23. domob1812 referenced this in commit b0aeba3fb4 on Mar 5, 2019
  24. jonasschnelli referenced this in commit 327d2746fb on Apr 8, 2019
  25. domob1812 commented at 7:56 am on April 8, 2019: contributor
    With #15495 merged, we can close this now.
  26. domob1812 closed this on Apr 8, 2019
  27. domob1812 deleted the branch on Apr 8, 2019
  28. domob1812 referenced this in commit b5c3195ea9 on Apr 8, 2019
  29. domob1812 referenced this in commit b43996135e on Apr 8, 2019
  30. jasonbcox referenced this in commit 4c94f3fa8c on Oct 5, 2020
  31. PastaPastaPasta referenced this in commit 9d7529e235 on Sep 11, 2021
  32. PastaPastaPasta referenced this in commit 7b451c4843 on Sep 11, 2021
  33. PastaPastaPasta referenced this in commit fc66b3f505 on Sep 12, 2021
  34. DrahtBot locked this on Dec 16, 2021


domob1812 promag laanwj practicalswift luke-jr kristapsk DrahtBot

Labels
RPC/REST/ZMQ

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-22 06:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me