R4R: Add timeouts to http servers by zmanian · Pull Request #2780 · tendermint/tendermint

zmanian · 2018-11-07T21:03:25Z

The current http server implementation does not enforce anytime outs on connections. This facilitates file descriptor exhaustion.

This introduces timeouts into the http server implementation.

… with ones with timeouts to prevent file descriptor exhaustion

zmanian · 2018-11-08T00:17:12Z

rpc/lib/server/http_server.go

-		logger.Info("RPC HTTP server stopped", "err", err)
+		s := &http.Server{
+			Handler:        RecoverAndLogHandler(maxBytesHandler{h: handler, n: maxBodyBytes}, logger),
+			ReadTimeout:    10 * time.Second,


Is 10 seconds too long?

I'd lower both to 3s

Suggested change

ReadTimeout: 10 * time.Second,

ReadTimeout: 3 * time.Second,

melekes · 2018-11-08T09:06:40Z

==================
WARNING: DATA RACE
Write at 0x00c420316270 by goroutine 31:
  github.com/tendermint/tendermint/rpc/lib/server.StartHTTPAndTLSServer.func1()
      /go/src/github.com/tendermint/tendermint/rpc/lib/server/http_server.go:112 +0x259

Previous write at 0x00c420316270 by goroutine 32:
  github.com/tendermint/tendermint/rpc/lib/server.StartHTTPAndTLSServer()
      /go/src/github.com/tendermint/tendermint/rpc/lib/server/http_server.go:119 +0xd7f
  github.com/tendermint/tendermint/rpc/lib/server.TestStartHTTPAndTLSServer()
      /go/src/github.com/tendermint/tendermint/rpc/lib/server/http_server_test.go:74 +0xe0
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:777 +0x16d

Goroutine 31 (running) created at:
  github.com/tendermint/tendermint/rpc/lib/server.StartHTTPAndTLSServer()
      /go/src/github.com/tendermint/tendermint/rpc/lib/server/http_server.go:105 +0xaa8
  github.com/tendermint/tendermint/rpc/lib/server.TestStartHTTPAndTLSServer()
      /go/src/github.com/tendermint/tendermint/rpc/lib/server/http_server_test.go:74 +0xe0
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:777 +0x16d

Goroutine 32 (running) created at:
  testing.(*T).Run()
      /usr/local/go/src/testing/testing.go:824 +0x564
  testing.runTests.func1()
      /usr/local/go/src/testing/testing.go:1063 +0xa4
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:777 +0x16d
  testing.runTests()
      /usr/local/go/src/testing/testing.go:1061 +0x4e1
  testing.(*M).Run()
      /usr/local/go/src/testing/testing.go:978 +0x2cd
  main.main()
      _testmain.go:112 +0x324
==================
--- FAIL: TestStartHTTPAndTLSServer (0.00s)
	Error Trace:	http_server_test.go:75
	Error:      	Expected nil, but got: &netutil.limitListener{Listener:(*net.TCPListener)(0xc4202c0040), sem:(chan struct {})(0xc4202c44e0), closeOnce:sync.Once{m:sync.Mutex{state:0, sema:0x0}, done:0x0}, done:(chan struct {})(0xc4202c4540)}
	Test:       	TestStartHTTPAndTLSServer
	testing.go:730: race detected during execution of test
FAIL

melekes

Awesome 💠 Thanks @zmanian

melekes · 2018-11-09T08:47:57Z

rpc/lib/server/http_server.go

-		logger.Info("RPC HTTP server stopped", "err", err)
+		s := &http.Server{
+			Handler:        RecoverAndLogHandler(maxBytesHandler{h: handler, n: maxBodyBytes}, logger),
+			ReadTimeout:    10 * time.Second,


I'd lower both to 3s

Suggested change

ReadTimeout: 10 * time.Second,

ReadTimeout: 3 * time.Second,

rpc/lib/server/http_server.go

jaekwon · 2018-11-10T00:03:39Z

This has magic constants in several places, and I'm not sure how it relates to other timeout parameters that we already have for peer connections. Needs a bit of review to parameterize & make consistent w/ the rest of the codebase.

jaekwon

See main thread

ebuchman · 2018-11-11T13:12:55Z

not sure how it relates to other timeout parameters that we already have for peer connections.

What does this have to do with peer connections? Are you saying we should make the http and p2p connection timeouts somehow related?

See main thread

What is this referring to ?

jaekwon · 2018-11-15T18:49:58Z

What does this have to do with peer connections? Are you saying we should make the http and p2p connection timeouts somehow related?

Ah no, we shouldn't do that. Lets just use constants instead of 10, and otherwise good to go.

Use variables for http timeouts

ebuchman · 2018-11-15T21:23:25Z

Oh - this breaks /broadcast_tx_commit. If committing the tx takes longer than the timeout here, then once the commit finally happens we get curl: (52) Empty reply from server.

Not sure how to reconcile ...

Does anyone know if/what the issue was here in the first place? Is there any endpoint that can hang for a while that we'd need this for?

@zmanian @jackzampolin

melekes · 2018-11-16T07:58:08Z

Oh - this breaks /broadcast_tx_commit. If committing the tx takes longer than the timeout here, then once the commit finally happens we get curl: (52) Empty reply from server.

Right. /broadcast_tx_commit timeout is 10 sec. now

tendermint/rpc/core/mempool.go

Line 197 in d8ab850

var deliverTxTimeout = 10 * time.Second // TODO: configurable?

. We can make it smaller or increase read/write timeouts (to 10 sec.).

melekes · 2018-11-16T08:00:10Z

rpc/lib/server/http_server.go

 	maxBodyBytes = int64(1000000) // 1MB
+
+	// same as the net/http default
+	maxHeaderBytes = 1 << 20


Go will use a default in the absence of such. Is there a reason to duplicate on our side?

Just to be explicit.

ebuchman · 2018-11-16T17:54:20Z

We can make it smaller or increase read/write timeouts (to 10 sec.)

We need to make one a function of the other so this doesn't become some kind of implicit dependency between constants.

I also still want to understand why we need these timeouts in the first place - under what circumstance are they actually important to us ?

ebuchman · 2018-11-17T00:38:17Z

Ok, I read https://blog.cloudflare.com/exposing-go-on-the-internet/ and https://blog.cloudflare.com/the-complete-guide-to-golang-net-http-timeouts/

It's clear we need to do this to handle hostile clients, but I still wonder if the original issue that prompted this might lie deeper than this fix, assuming our clients were functioning properly. I suspect there are other issues with file descriptors in Tendermint, but anyways.

@zmanian @melekes PTAL.

The `broadcast_tx_commit` endpoint has it's own timeout. If this is longer than the http server's WriteTimeout, the user will receive an error. Here, we export the WriteTimeout and set the broadcast_tx_commit timeout to be less than it. In the future, we should use a config struct for the timeouts to avoid the need to export. The broadcast_tx_commit timeout may also become configurable, but we must check that it's less than the server's WriteTimeout.

codecov-io · 2018-11-17T00:43:26Z

Codecov Report

Merging #2780 into develop will increase coverage by 0.03%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           develop    #2780      +/-   ##
===========================================
+ Coverage    62.09%   62.13%   +0.03%     
===========================================
  Files          212      212              
  Lines        17279    17283       +4     
===========================================
+ Hits         10730    10738       +8     
+ Misses        5642     5639       -3     
+ Partials       907      906       -1

Impacted Files	Coverage Δ
rpc/core/pipe.go	`30.23% <ø> (ø)`	⬆️
rpc/core/mempool.go	`0% <0%> (ø)`	⬆️
privval/tcp_server.go	`78.57% <0%> (-2.86%)`	⬇️
evidence/reactor.go	`63.44% <0%> (+0.81%)`	⬆️
libs/flowrate/flowrate.go	`82.35% <0%> (+1.61%)`	⬆️
privval/ipc_server.go	`69.81% <0%> (+5.66%)`	⬆️

zmanian · 2018-11-17T01:07:43Z

I think the other file descriptor issues might a result of leveldb settings

melekes

💠

@odeke-em

…the JSON (backport tendermint#2774) (tendermint#2780) This change fixes a bug in which BitArray.UnmarshalJSON hadn't accounted for the fact that invoking NewBitArray(<=0) returns nil and hence when dereferenced would crash with a runtime nil pointer dereference. This bug was found by my security analysis and fuzzing too. Author: @odeke-em Fixes cometbft/cometbft#2658 --- #### PR checklist - [x] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] ~~Updated relevant documentation (`docs/` or `spec/`) and code comments~~ - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec <hr>This is an automatic backport of pull request tendermint#2774 done by [Mergify](https://mergify.com). --------- Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com> Co-authored-by: Andy Nogueira <me@andynogueira.dev>

Replaces our current http servers where connections stay open forever…

a5dad9d

… with ones with timeouts to prevent file descriptor exhaustion

zmanian requested review from ebuchman, melekes and xla as code owners November 7, 2018 21:03

zmanian changed the base branch from master to develop November 7, 2018 21:03

Zaki Manian added 2 commits November 7, 2018 13:37

Use the correct handler

dea4890

Put in go routines

3b7bff5

zmanian changed the title ~~Zaki/http server with timeouts~~ R4R: Zaki/http server with timeouts Nov 7, 2018

zmanian changed the title ~~R4R: Zaki/http server with timeouts~~ R4R: Add timeouts to http servers Nov 7, 2018

zmanian commented Nov 8, 2018

View reviewed changes

melekes reviewed Nov 9, 2018

View reviewed changes

jackzampolin mentioned this pull request Nov 9, 2018

Game of Stakes issue list #2794

Closed

6 tasks

jaekwon requested changes Nov 10, 2018

View reviewed changes

jaekwon approved these changes Nov 15, 2018

View reviewed changes

ebuchman added 3 commits November 15, 2018 16:07

Merge branch 'develop' into zaki/HttpServerWithTimeouts

56e6f11

Use variables for http timeouts

fix err

ff4b6aa

changelog

6620dfc

melekes reviewed Nov 16, 2018

View reviewed changes

Merge branch 'develop' into zaki/HttpServerWithTimeouts

e39673a

melekes approved these changes Nov 17, 2018

View reviewed changes

ebuchman merged commit e6fc10f into tendermint:develop Nov 17, 2018

Conversation

zmanian commented Nov 7, 2018

Uh oh!

zmanian Nov 8, 2018

Choose a reason for hiding this comment

Uh oh!

melekes Nov 9, 2018

Choose a reason for hiding this comment

Uh oh!

melekes commented Nov 8, 2018

Uh oh!

melekes left a comment

Choose a reason for hiding this comment

Uh oh!

melekes Nov 9, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jaekwon commented Nov 10, 2018

Uh oh!

jaekwon left a comment

Choose a reason for hiding this comment

Uh oh!

ebuchman commented Nov 11, 2018

Uh oh!

jaekwon commented Nov 15, 2018

Uh oh!

ebuchman commented Nov 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

melekes commented Nov 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

melekes Nov 16, 2018

Choose a reason for hiding this comment

Uh oh!

ebuchman Nov 16, 2018

Choose a reason for hiding this comment

Uh oh!

ebuchman commented Nov 16, 2018

Uh oh!

ebuchman commented Nov 17, 2018

Uh oh!

codecov-io commented Nov 17, 2018

Codecov Report

Uh oh!

zmanian commented Nov 17, 2018

Uh oh!

melekes left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ebuchman commented Nov 15, 2018 •

edited

Loading

melekes commented Nov 16, 2018 •

edited

Loading