Replace `tokio_core` with `tokio` (`ring` -> 0.13) #9657

c0gent · 2018-09-26T22:42:46Z

Changes

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.

Still Incomplete

Related Upcoming TODOs (separate PR)

Convert many boxed future return types to impl Future and clean up extra types.
Ensure no errors are discarded in secret_store/src/key_server_cluster/cluster.rs (and elsewhere).
Remove or modify the it_should_not_read_too_much_data_sync test (util/fetch/src/client.rs).

Closes #9533
Closes #9722

parity-cla-bot · 2018-09-26T22:42:48Z

It looks like @c0gent signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

ordian · 2018-09-27T07:34:51Z

Cargo.toml

-ring = { git = "https://github.com/paritytech/ring" }
+# ring = { git = "https://github.com/paritytech/ring" }
+ring = { git = "https://github.com/briansmith/ring" }
+parity-crypto = { git = "https://github.com/c0gent/parity-common", branch = "ring-0.13" }


parity-crypto = "0.2"

ordian · 2018-09-27T07:35:45Z

Cargo.toml

@@ -140,4 +140,7 @@ members = [
 ]

 [patch.crates-io]
-ring = { git = "https://github.com/paritytech/ring" }
+# ring = { git = "https://github.com/paritytech/ring" }
+ring = { git = "https://github.com/briansmith/ring" }


we don't need this

tomaka · 2018-09-27T10:23:44Z

Cargo.toml

+# ring = { git = "https://github.com/paritytech/ring" }
+ring = { git = "https://github.com/briansmith/ring" }
+parity-crypto = { git = "https://github.com/c0gent/parity-common", branch = "ring-0.13" }
+cid = { git = "https://github.com/c0gent/rust-cid", branch = "bump-multihash" }


I pinged the slack of Protocol Labs about multiformats/rust-cid#11

tomaka · 2018-09-27T10:24:59Z

ipfs/src/error.rs

+}
+
+impl ::std::error::Error for ServerError {
+	fn description(&self) -> &str { "ServerError" }


The methods of the Error trait are deprecated. Instead, it's the implementation of Display that should write ServerError.

Indeed, (and I'm inclined to move errors towards failure depending on what happens with the new std error trait) but std::error::Error is required when building a hyper server.

Error itself is not deprecated, just implementing the methods on it.
You still should write impl Error for ServerError {}.

Ah, my mistake. It's been a while since I've manually implemented it at all. :)

I thought maybe you were referring to potential future deprecation: rust-lang/rfcs#2504.

tomaka · 2018-09-27T10:26:42Z

secret_store/src/key_server_cluster/cluster.rs

@@ -59,7 +59,7 @@ const KEEP_ALIVE_SEND_INTERVAL: Duration = Duration::from_secs(30);
 const KEEP_ALIVE_DISCONNECT_INTERVAL: Duration = Duration::from_secs(60);

 /// Empty future.
-pub type BoxedEmptyFuture = Box<Future<Item = (), Error = ()> + Send>;
+pub type BoxedEmptyFuture = Box<Future<Item = (), Error = ()> + Send + 'static>;


Nit: change shouldn't be needed.

tomaka · 2018-09-27T10:27:18Z

secret_store/src/key_server_cluster/cluster.rs


 		Ok(Arc::new(ClusterCore {
-			handle: handle,
+			// executor: executor,


Commented out code.

tomaka · 2018-09-27T10:28:30Z

secret_store/src/key_server_cluster/cluster.rs

-			}
-		}
+		let check_predicate = future::loop_fn((), move|_| {
+				if predicate() {


Wrong indentation.

tomaka · 2018-09-27T10:28:37Z

secret_store/src/key_server_cluster/cluster.rs

@@ -1119,7 +1117,15 @@ pub mod tests {
 	use std::time::{Duration, Instant};
 	use std::collections::{BTreeSet, VecDeque};
 	use parking_lot::Mutex;
-	use tokio_core::reactor::Core;
+	use tokio::{
+		// reactor::Reactor,


Commented out code.

tomaka · 2018-09-27T10:29:25Z

secret_store/src/key_server_cluster/io/deadline.rs

+pub fn deadline<F, T>(duration: Duration, future: F) -> Result<Deadline<F>, io::Error>
+	where F: Future<Item = T, Error = io::Error> + Send + 'static, T: Send + 'static
+{
+	// let timeout: DeadlineBox<F> = Box::new(Timeout::new((), duration)?.map(|_| DeadlineStatus::Timeout));


Commented out code.

dvdplm

Good stuff here, I like it! :)

dvdplm · 2018-10-03T08:02:55Z

Cargo.toml

@@ -31,7 +31,7 @@ futures = "0.1"
 futures-cpupool = "0.1"
 fdlimit = "0.1"
 ctrlc = { git = "https://github.com/paritytech/rust-ctrlc.git" }
-jsonrpc-core = { git = "https://github.com/paritytech/jsonrpc.git", branch = "parity-1.11" }
+jsonrpc-core = { git = "https://github.com/c0gent/jsonrpc.git", branch = "c0gent-hyper" }


This will change back to "https://github.com/paritytech/jsonrpc.git" at some point before the PR lands right?

Absolutely. Currently just waiting on a specific release/branch/tag etc. to depend on (It's probably wise to hold off on that until this PR is confirmed to work properly).

dvdplm · 2018-10-03T08:03:41Z

ethcore/stratum/Cargo.toml

-jsonrpc-tcp-server = { git = "https://github.com/paritytech/jsonrpc.git", branch = "parity-1.11" }
+jsonrpc-core = { git = "https://github.com/c0gent/jsonrpc.git", branch = "c0gent-hyper" }
+jsonrpc-macros = { git = "https://github.com/c0gent/jsonrpc.git", branch = "c0gent-hyper" }
+jsonrpc-tcp-server = { git = "https://github.com/c0gent/jsonrpc.git", branch = "c0gent-hyper" }


These will change back to "https://github.com/paritytech/jsonrpc.git" at some point before the PR lands right?

dvdplm · 2018-10-03T08:08:05Z

rpc/src/tests/ws.rs

@@ -34,10 +34,9 @@ pub fn serve() -> (Server<ws::Server>, usize, GuardedAuthCodes) {
 	let authcodes = GuardedAuthCodes::new();
 	let stats = Arc::new(informant::RpcStats::default());

-	let res = Server::new(|remote| ::start_ws(
+	let res = Server::new(|_remote| ::start_ws(


Why not just Server::new(|_| …?

No reason really. I like to label those unused variables just to make it easy to know what's there but I'm happy to change it.

dvdplm · 2018-10-03T08:09:04Z

rpc/src/v1/extractors.rs

-		X: core::futures::Future<Item=Option<core::Response>, Error=()> + Send + 'static,
+	fn on_request<F, X>(&self, request: core::Request, meta: Metadata, process: F)
+		-> Either<Self::Future, X>
+		where F: FnOnce(core::Request, Metadata) -> X,


Funny indentation here.

I'm a funny guy!

dvdplm · 2018-10-03T08:11:03Z

secret_store/src/key_server_cluster/cluster.rs

@@ -137,7 +137,7 @@ pub struct ClusterConfiguration {
 	/// Administrator public key.
 	pub admin_public: Option<Public>,
 	/// Should key servers set change session should be started when servers set changes.
-	/// This will only work when servers set is configured using KeyServerSet contract.
+	/// This will only work when servers set is configured  using KeyServerSet contract.


Double spaces here. Also: s/when servers set is/when the server set is/

dvdplm · 2018-10-03T08:12:13Z

secret_store/src/key_server_cluster/cluster.rs

-	/// Handle to the event loop.
-	handle: Handle,
+	// /// Handle to the event loop.
+	// executor: TaskExecutor,


commented out lines

dvdplm · 2018-10-03T08:28:50Z

secret_store/src/key_server_cluster/cluster.rs

 	}

 	/// Start listening for incoming connections.
-	fn listen(handle: &Handle, data: Arc<ClusterData>, listen_address: SocketAddr) -> Result<BoxedEmptyFuture, Error> {
-		Ok(Box::new(TcpListener::bind(&listen_address, &handle)?
+	fn listen(data: Arc<ClusterData>, listen_address: SocketAddr) -> Result<BoxedEmptyFuture, Error> {


I haven't traced through all the calls but aren't we dropping a few possible errors in this method? Perhaps out of scope for this PR though...

Yes, the errors are not handled the way I would do it. I've tried not to change too much functionality from the original in this PR in order to keep things as straightforward as possible. I'll make a note to have a look at that in an upcoming PR :)

@dvdplm Not sure I got it - all connection-establish (both for incoming && outcoming) related errors are processed in process_connection_result by tracing? Probably some other errors are dropped? Or do you have idea how to handle these errors in other way? Could you please, elaborate?

secret_store/src/key_server_cluster/cluster.rs

util/fetch/Cargo.toml

c0gent · 2018-10-03T19:17:40Z

I haven't been able to spend time on it but I could still use help from someone more familiar with json_tests to give me some idea of where I might start looking for the reason why those are failing.

dvdplm · 2018-10-09T14:53:02Z

re-json_tests: @sorpaas are you able to provide a few pointers here? :)

ethcore/Cargo.toml

sorpaas · 2018-10-09T15:27:34Z

@c0gent The issue looks like that you updated ethereum/tests submodule in this PR. Revert back that part should fix jsontests.

c0gent · 2018-10-09T16:18:57Z

I'm re-running the tests and I have to leave for an appointment. I'll push the changes this afternoon (PST).

It will leave two items remaining (see OP), 1.) A jsonrpc tag/branch/release to depend on, 2.) Decide what to do about that test -- this can be deferred to a future PR.

cheme · 2018-10-09T19:36:34Z

About the json_tests, as @sorpaas point out, it is a submodule issue (I just run them successfully with your PR).
About jsonrpc, I ran the test with latest master dependencies, and got some failures, mainly related to secret_store keyserver:

failures:
    key_server_cluster::client_sessions::generation_session::tests::encryption_session_works_over_network
    key_server_cluster::cluster::tests::error_in_generation_session_broadcasted_to_all_other_nodes
    key_server_cluster::cluster::tests::generation_session_completion_signalled_if_failed_on_master
    key_server_cluster::cluster::tests::generation_session_is_removed_when_succeeded
    key_server_cluster::cluster::tests::sessions_are_removed_when_initialization_fails

Plus stratum 'can_push_work' also seem to fail, but mainly when running with other tests (randomly).
But others than those 6 tests all seems to pass :)

c0gent · 2018-10-09T19:41:16Z

Yup thanks. I'll fix all of that when I get back in a few hrs (hopefully).

c0gent · 2018-10-10T01:23:12Z

I can't reproduce those CI test failures locally but I'll see if I can get to the bottom of it. It's undoubtedly related to the ungraceful shutdown issue noted above.

5chdn · 2018-10-19T11:58:01Z

Will merge once last grumbles are addressed

tomusdrw · 2018-10-19T07:40:54Z

Cargo.toml

 fdlimit = "0.1"
 ctrlc = { git = "https://github.com/paritytech/rust-ctrlc.git" }
-jsonrpc-core = { git = "https://github.com/paritytech/jsonrpc.git", branch = "parity-1.11" }
+jsonrpc-core = { git = "https://github.com/paritytech/jsonrpc.git", rev = "7a59776" }


I branched out jsonrpc now, can you update this to use parity-2.2 branch?

secret_store/src/key_server_cluster/cluster.rs

tomusdrw · 2018-10-19T09:43:58Z

secret_store/src/key_server_cluster/cluster.rs

@@ -672,7 +675,7 @@ impl ClusterConnections {
 		Ok(ClusterConnections {
 			self_node_id: config.self_key_pair.public().clone(),
 			key_server_set: config.key_server_set.clone(),
-			trigger: Mutex::new(trigger),
+			trigger: RwLock::new(trigger),


Seems we never do self.trigger.read() anyway, what's the purpose of this change?

Good point, changed back.

tomusdrw · 2018-10-19T09:55:59Z

secret_store/src/key_server_cluster/cluster.rs

-				panic!("no result in {:?}", timeout);
-			}
-		}
+		runtime.block_on(Interval::new_interval(Duration::new(0, 1_000_000))


Can you just use Duration::from_millis(1) instaed of specifying nanoseconds?

Not sure why I did that, I usually do use from_millis.

secret_store/src/key_server_cluster/cluster.rs

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

c0gent · 2018-10-19T16:29:15Z

Should be all addressed.

niklasad1 · 2018-10-20T11:12:46Z

@tomusdrw are you ok with this now? if that's the case let's merge it

tomusdrw · 2018-10-20T11:39:14Z

Looks good but the tests are failing and some of the failures seems legit. @5chdn is it a known issue or something specific to this PR?

5chdn · 2018-10-22T07:38:35Z

Failures are unrelated. Thanks, everyone.

5chdn · 2018-10-22T07:38:48Z

Failures are unrelated. Thanks, everyone.

5chdn · 2018-10-23T09:19:54Z

Merged 4x 🤦‍♂️

ordian reviewed Sep 27, 2018

View reviewed changes

5chdn added this to the 2.2 milestone Sep 27, 2018

5chdn added A0-pleasereview 🤓 Pull request needs code review. A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M5-dependencies 🖇 Dependencies. labels Sep 27, 2018

tomaka reviewed Sep 27, 2018

View reviewed changes

tomaka previously requested changes Sep 27, 2018

View reviewed changes

c0gent force-pushed the c0gent-tokio branch 2 times, most recently from b4e8e4d to bd7d453 Compare September 27, 2018 20:31

5chdn removed the A0-pleasereview 🤓 Pull request needs code review. label Sep 27, 2018

c0gent force-pushed the c0gent-tokio branch 2 times, most recently from 2fe3c98 to 4a49a06 Compare September 28, 2018 16:39

dvdplm reviewed Oct 3, 2018

View reviewed changes

c0gent force-pushed the c0gent-tokio branch from 4a49a06 to 73502eb Compare October 3, 2018 19:16

c0gent force-pushed the c0gent-tokio branch 3 times, most recently from 0c9c31a to 8f087b6 Compare October 8, 2018 22:42

ordian mentioned this pull request Oct 9, 2018

error: Vulnerable crates found! #9722

Closed

sorpaas reviewed Oct 9, 2018

View reviewed changes

ethcore/Cargo.toml Show resolved Hide resolved

c0gent force-pushed the c0gent-tokio branch from 8f087b6 to 1954cbf Compare October 9, 2018 23:40

c0gent force-pushed the c0gent-tokio branch from 1954cbf to d4f8d40 Compare October 10, 2018 02:02

5chdn added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 19, 2018

tomusdrw reviewed Oct 19, 2018

View reviewed changes

ordian mentioned this pull request Oct 19, 2018

deps: update jsonrpc-core #9780

Closed

c0gent force-pushed the c0gent-tokio branch from 066c74f to 59c1d23 Compare October 19, 2018 16:27

svyatonik added A0-pleasereview 🤓 Pull request needs code review. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Oct 19, 2018

ordian approved these changes Oct 19, 2018

View reviewed changes

ordian removed the A0-pleasereview 🤓 Pull request needs code review. label Oct 19, 2018

tomusdrw approved these changes Oct 20, 2018

View reviewed changes

tomusdrw added the A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. label Oct 20, 2018

5chdn merged commit 15d71a0 into openethereum:master Oct 23, 2018

This was referenced Oct 23, 2018

Backports: parity stable 2.0.9 #9786

Merged

Client doesn't shutdown gracefully on Ctrl-C #9807

Closed

This was referenced Oct 26, 2018

ci: remove failing tests for android, windows, and macos #9788

Merged

Android test builds do not compile #9721

Closed

ordian mentioned this pull request Oct 30, 2018

Deduplicate smallvec dependency #9558

Closed

tomusdrw mentioned this pull request Nov 15, 2018

Clarify jsonrpc threads parameters #9260

Closed

c0gent mentioned this pull request Nov 22, 2018

Remove runtime from secret_store. #9888

Merged

5chdn mentioned this pull request Nov 27, 2018

Backports for stable 2.1.7 #9975

Merged

10 tasks

grbIzl mentioned this pull request Nov 28, 2018

Add tokio runtime to ethcore io worker #9979

Merged

dvdplm mentioned this pull request Nov 18, 2019

Ensure jsonrpc threading settings are sane #11267

Merged

Replace tokio_core with tokio (ring -> 0.13) #9657

Replace tokio_core with tokio (ring -> 0.13) #9657

Conversation

c0gent commented Sep 26, 2018 • edited Loading

Changes