[Feature] Enable http-keepalives/HTTP2 on JSON-RPC interface!

[zquestz]

Enable support for http-keepalive's on the JSON-RPC interface. In addition we gain HTTP2 support by getting rid of the hijack code!

I have tested this extensively and it works great with curl/wget/bchctl. I also was able to mitigate the issue that required the hijack code. By setting the ReadHeaderTimeout it can check the RPC auth and then allow an auth'd client to use the ReadTimeout for long poll requests.

I also updated the code to use the built in http CloseNotifier.

Fixes: #380

[cculianu]

Hmm I just tried it. I telnetted to it and hand-crafted the header & request.. After issuing it a few "server.pings" in succession I consistently get a panic. I also tried Fulcrum and it started the synch process only for bchd to also panic immediately.

This is the stack trace each time, same trace:

panic: net/http: CloseNotify called after ServeHTTP finished

goroutine 8783 [running]:
net/http.(*response).CloseNotify(0xc0006d61c0, 0x1be0160)
	/opt/local/lib/go/src/net/http/server.go:1972 +0x5e
main.(*rpcServer).jsonRPCRead.func1(0x1be0160, 0xc0006d61c0, 0xc01173b4a0)
	/Users/calin/src/bchd/rpcserver.go:4275 +0x55
created by main.(*rpcServer).jsonRPCRead
	/Users/calin/src/bchd/rpcserver.go:4274 +0x153

[zquestz]

Status update, appears to be working. No more panics. =)

[cculianu]

Awesome. One more thing not related to this change -- the other bitcoind's expect a boolean value for getrawmempool's first argument, but bchd panics if it is a boolean and complains about expecting int:

{"error":{"code":-8,"message":"Failed to parse request: parameter #2 'verbose' must be type int (got bool)"},"id":168,"jsonrpc":"1.0","result":null}

Shall I open an issue for this?

[cculianu]

Oh.. I see. This is not an issue. I had the bitcoind RPCQuirks variable set in the bchd.conf. Weird. I grepped for that and found no mention of it in the codebase. If the variable is not set, this error goes away.

[cculianu]

Oh no.. my bad. Error still happens. Weird. So getrawmempool's first arg (verbose arg) is a boolean, true/false, but getrawtransaction's second arg (also boolean true/false) expects an int. :(

[zquestz]

Awesome. One more thing not related to this change -- the other bitcoind's expect a boolean value for getrawmempool's first argument, but bchd panics if it is a boolean and complains about expecting int:

{"error":{"code":-8,"message":"Failed to parse request: parameter #2 'verbose' must be type int (got bool)"},"id":168,"jsonrpc":"1.0","result":null}

Shall I open an issue for this?

Wait, it panics or just returns an error?

[zquestz]

Yeah... getrawtransaction takes 2 args, one is the tx id, then verbose flag which is indeed an int. However, what worries me is that you are seeing panics... trying to replicate that now.

[zquestz]

So the only thing I am really worried about here is the panic's you reported that I can't replicate. I was curling my node and can't simulate any panics. If I pass the wrong type it complains but does not crash the process. Can you share a call that causes the panic?

[zquestz]

NOTE: there were no panics, @cculianu was just saying it threw an error. Nothing to worry about crashing wise. =)

[cculianu]

@zquestz Yeah sorry for using the panic word in a panicky way. I meant the English sense of the word. I hardly program much Go so to me that word has no special panic-inducing connotation associated with it....

[cculianu]

BTW I have been running Fulcrum connected to bchd for hours now in a test and after setting the ping timer (on the Fulcrum side) to something like 3 seconds (paranoia).. it never drops conn now due to idle.. and it works very fast. This is off the latest commit as of the time of this writing: d1da409

I'm all-for this feature!

[cculianu]

Do you think there's a strong chance this change (or something equivalent) will make it into mainline bchd? I want to bullet-point in my next fulcrum release that "bchd support is ready!"... but I don't know if that's premature as of now.

Thanks.