Fix threading and closing of servers #495

tomusdrw · 2019-10-03T11:54:35Z

This PRs attempts to clean up the spawned threads (Closes #425).
Currently every RpcEventLoop spawns a bunch of threads that depend on the number of cores (tokio runtime) + one thread that waits for the runtime to complete. Instead we just spawn tokio::runtime with one thread and a right name.

Also we avoid calling tokio::spawn when handling incoming connections in various server implementations to ensure that when the server is closed all connections are dropped as well (currently, they are still being polled by the runtime so you have to wait for them to close or timeout).

ordian

Nice! Would be nice to have a test for a clean shutdown to avoid regressions in the future.

tomusdrw · 2019-10-04T11:32:25Z

@ordian good point, let me try to come up with something.

dvdplm

I think this is not ok. Not sure exactly why, but it seems to handle a single req at a time (which amounts to a weird experience when using e.g. MyCrypto that holds on to a connection for the whole session: all other requests block).

dvdplm · 2019-10-04T14:50:50Z

Not sure exactly what change causes this but when parity-ethereum shuts down, this is printed:

^C2019-10-04 16:25:21  main INFO parity_ethereum::run  Finishing work, please wait...
2019-10-04 16:25:21  http.worker00 WARN jsonrpc_server_utils::suspendable_stream  Error accepting connection: reactor gone
2019-10-04 16:25:21  http.worker00 WARN jsonrpc_server_utils::suspendable_stream  The server will stop accepting connections for 20ms

Not a big deal but maybe we can dial it down from warn to debug?

dvdplm

LGTM.

I think it would be useful to have some module level docs that clearly states how threads are configured. And a test. ;)

dvdplm · 2019-10-04T14:52:28Z

http/examples/http_async.rs

@@ -9,6 +9,7 @@ fn main() {

 	let server = ServerBuilder::new(io)
 		.cors(DomainsValidation::AllowOnly(vec![AccessControlAllowOrigin::Null]))
+		.threads(8)


Suggested change

.threads(8)

.threads(4)

Maybe a saner default?

Removed this altogether, pushed by mistake :)

tomusdrw · 2019-10-04T20:50:09Z

@ordian @dvdplm Added a test that fails on master (the main issue is actually in dropping IoHandler not the server itself) and some docs to reactor module and ServerBuilder::threads method.

dvdplm · 2019-10-05T06:52:58Z

http/src/tests.rs

+		}
+		std::thread::sleep(std::time::Duration::from_millis(10));
+	}
+	assert!(false);


Suggested change

assert!(false);

assert!(false, "expected server to be closed");

Why not panic directly?

dvdplm · 2019-10-05T06:55:21Z

Travis failure seems spurious (restarted) but appveyor is perhaps legit?

ordian · 2019-10-06T09:35:04Z

Let's merge this?

dvdplm · 2019-10-06T10:49:43Z

Let's merge this?

I think we should sort out the Travis failures first.

ordian · 2019-10-06T11:28:11Z

@dvdplm I though it was spurious, can you confirm the test fails on macOS?

dvdplm · 2019-10-07T11:26:18Z

I have one failing test in jsonrpc-derive. Is that only osx though? @ordian/@todr can you try too please?

ordian · 2019-10-07T11:57:32Z

cargo test --all passes on my linux machine

tomusdrw · 2019-10-07T12:04:42Z

All good here as well. I just restarted the failing unix/beta build. But I think it should be good to go, all the previous failures where spurious.

tomusdrw · 2019-10-07T12:32:02Z

Oh, actually it depends on the rust compiler version. Just bumped my compiler to 1.38 and seeing issues with compile_test test.

Contains paritytech/jsonrpc#495 with good bugfixes to resource usage.

* Upgrade to jsonrpc v14 Contains paritytech/jsonrpc#495 with good bugfixes to resource usage. * Bump tokio & futures. * Bump even further. * Upgrade tokio to 0.1.22 * Partially revert "Bump tokio & futures." This reverts commit 100907e.

* Upgrade to jsonrpc v14 Contains paritytech/jsonrpc#495 with good bugfixes to resource usage. * Bump tokio & futures. * Bump even further. * Upgrade tokio to 0.1.22 * Partially revert "Bump tokio & futures." This reverts commit 100907eb91907aa124d856d52374637256118e86.

) * Replace `tokio_core` with `tokio` (`ring` -> 0.13) (#9657) * Replace `tokio_core` with `tokio`. * Remove `tokio-core` and replace with `tokio` in - `ethcore/stratum` - `secret_store` - `util/fetch` - `util/reactor` * Bump hyper to 0.12 in - `miner` - `util/fake-fetch` - `util/fetch` - `secret_store` * Bump `jsonrpc-***` to 0.9 in - `parity` - `ethcore/stratum` - `ipfs` - `rpc` - `rpc_client` - `whisper` * Bump `ring` to 0.13 * Use a more graceful shutdown process in `secret_store` tests. * Convert some mutexes to rwlocks in `secret_store`. * Consolidate Tokio Runtime use, remove `CpuPool`. * Rename and move the `tokio_reactor` crate (`util/reactor`) to `tokio_runtime` (`util/runtime`). * Rename `EventLoop` to `Runtime`. - Rename `EventLoop::spawn` to `Runtime::with_default_thread_count`. - Add the `Runtime::with_thread_count` method. - Rename `Remote` to `Executor`. * Remove uses of `CpuPool` and spawn all tasks via the `Runtime` executor instead. * Other changes related to `CpuPool` removal: - Remove `Reservations::with_pool`. `::new` now takes an `Executor` as an argument. - Remove `SenderReservations::with_pool`. `::new` now takes an `Executor` as an argument. * Remove secret_store runtimes. (#9888) * Remove the independent runtimes from `KeyServerHttpListener` and `KeyServerCore` and instead require a `parity_runtime::Executor` to be passed upon creation of each. * Remove the `threads` parameter from both `ClusterConfiguration` structs. * Implement the `future::Executor` trait for `parity_runtime::Executor`. * Update tests. - Update the `loop_until` function to instead use a oneshot to signal completion. - Modify the `make_key_servers` function to create and return a runtime. * misc: bump license header to 2019 (#10135) * misc: bump license header to 2019 * misc: remove_duplicate_empty_lines.sh * misc: run license header script * commit cargo lock * Upgrade to jsonrpc v14 (#11151) * Upgrade to jsonrpc v14 Contains paritytech/jsonrpc#495 with good bugfixes to resource usage. * Bump tokio & futures. * Bump even further. * Upgrade tokio to 0.1.22 * Partially revert "Bump tokio & futures." This reverts commit 100907eb91907aa124d856d52374637256118e86. * Added README, CHANGELOG and several meta tags in Cargo.toml * Proper pr in changelog * Remove categories tag * Comments and usage fixed * Declare test usage for methods explicitly * Crate name in readme modified, complete removed * Test helpers feature added, functions marked as test only * Processed by fmt tool * Illustrative example added * Sample moved into the separate directory * use examples directory instead of custom crate * Wait till scanning completed * Timeout decreased * Unused methods removed Co-authored-by: Nick Sanders <[email protected]> Co-authored-by: 5chdn <[email protected]> Co-authored-by: David <[email protected]> Co-authored-by: Nikolay Volf <[email protected]>

tomusdrw added 2 commits October 3, 2019 13:11

Remove unnecessary spawns.

12e3431

Fix how we manage threads.

b76eba1

tomusdrw requested a review from dvdplm October 3, 2019 11:54

Remove unused import.

18d320d

ordian approved these changes Oct 4, 2019

View reviewed changes

dvdplm suggested changes Oct 4, 2019

View reviewed changes

Parallelize incoming connections.

6d25cf2

dvdplm approved these changes Oct 4, 2019

View reviewed changes

tomusdrw added 2 commits October 4, 2019 22:41

Add close server test.

ae8ffd5

Add some docs.

240b92f

Tone down warnings.

b8b7db5

dvdplm reviewed Oct 5, 2019

View reviewed changes

Fix test assert.

128b109

dvdplm mentioned this pull request Oct 7, 2019

Memory usage spike induced crash openethereum/parity-ethereum#10371

Closed

tomusdrw merged commit 3b790c6 into master Oct 7, 2019

tomusdrw deleted the td-tokio branch October 7, 2019 12:48

tomusdrw mentioned this pull request Oct 7, 2019

Bump to 14.0 #496

Merged

dvdplm added a commit to openethereum/parity-ethereum that referenced this pull request Oct 9, 2019

Upgrade to jsonrpc v14

f8c3232

Contains paritytech/jsonrpc#495 with good bugfixes to resource usage.

dvdplm mentioned this pull request Oct 9, 2019

Upgrade to jsonrpc v14 openethereum/parity-ethereum#11151

Merged

quake mentioned this pull request Dec 23, 2019

feat: add tcp and websocket to rpc service nervosnetwork/ckb#1882

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix threading and closing of servers #495

Fix threading and closing of servers #495

tomusdrw commented Oct 3, 2019

ordian left a comment

tomusdrw commented Oct 4, 2019

dvdplm left a comment

dvdplm commented Oct 4, 2019

dvdplm left a comment

dvdplm Oct 4, 2019

tomusdrw Oct 4, 2019

tomusdrw commented Oct 4, 2019

dvdplm Oct 5, 2019

bkchr Oct 5, 2019

dvdplm commented Oct 5, 2019

ordian commented Oct 6, 2019

dvdplm commented Oct 6, 2019

ordian commented Oct 6, 2019

dvdplm commented Oct 7, 2019

ordian commented Oct 7, 2019

tomusdrw commented Oct 7, 2019

tomusdrw commented Oct 7, 2019

	assert!(false);
	assert!(false, "expected server to be closed");

Fix threading and closing of servers #495

Fix threading and closing of servers #495

Conversation

tomusdrw commented Oct 3, 2019

ordian left a comment

Choose a reason for hiding this comment

tomusdrw commented Oct 4, 2019

dvdplm left a comment

Choose a reason for hiding this comment

dvdplm commented Oct 4, 2019

dvdplm left a comment

Choose a reason for hiding this comment

dvdplm Oct 4, 2019

Choose a reason for hiding this comment

tomusdrw Oct 4, 2019

Choose a reason for hiding this comment

tomusdrw commented Oct 4, 2019

dvdplm Oct 5, 2019

Choose a reason for hiding this comment

bkchr Oct 5, 2019

Choose a reason for hiding this comment

dvdplm commented Oct 5, 2019

ordian commented Oct 6, 2019

dvdplm commented Oct 6, 2019

ordian commented Oct 6, 2019

dvdplm commented Oct 7, 2019

ordian commented Oct 7, 2019

tomusdrw commented Oct 7, 2019

tomusdrw commented Oct 7, 2019