Update jsonrpc dependencies and rewrite dapps to futures. #6522

tomusdrw · 2017-09-15T11:03:57Z

The HTTP (RPC/Dapps & UI) servers are now based on latest version of hyper (which also makes them way more performant).
minihttp is not needed any more, http supports multiple server threads as well.
Updates futures to latest version and removes all .boxed() and BoxFuture occurrences (deprecated).

~~Awaiting for paritytech/jsonrpc#199 to be reviewed and merged first (so that we can branch out as parity-1.8)~~

Closes #6261, #6476

gavofyork · 2017-09-18T06:55:20Z

doesn't build..

debris

overall, looks fine, but the pr of that size is simply unreviewable

debris · 2017-09-30T07:48:45Z

Cargo.toml

@@ -117,4 +117,5 @@ lto = false
 panic = "abort"

 [workspace]
-members = ["ethstore/cli", "ethkey/cli", "evmbin", "whisper", "chainspec", "dapps/node-health"]
+members = ["ethstore/cli", "ethkey/cli", "evmbin", "whisper", "chainspec"]
+exclude = ["dapps/js-glue"]


why is it excluded?

debris · 2017-09-30T07:59:41Z

dapps/src/handlers/fetch.rs

+				FetchState::Done(_, ref mut response) => {
+					return response.poll()
+				},
+				_ => unreachable!(),


it's reachable by FetchState::Empty

debris · 2017-09-30T08:03:36Z

dapps/src/handlers/fetch.rs

+				FetchState::Streaming(response) => {
+					return Ok(futures::Async::Ready(response));
+				},
+				any => any,


imo, it's better to write:

if let FetchState::Error(ref mut error) = self.status { // here do mem::replace on error // and return Ok(..) } if let FetchState::Streaming(ref mut response) = self.response { // here do mem::replace on response // and return Ok(..) }

That doesn't give you the ownership of error required for return.

debris · 2017-09-30T08:32:15Z

dapps/src/handlers/reader.rs

+
+const MAX_CHUNK_SIZE: usize = 32 * 1024;
+
+pub struct Reader<R: io::Read> {


This structure requires very descriptive documentation. I've spent over 10 minutes looking at it to understand why it's done like that.

And if I understand correctly, sending is a sink, which sends data directly to a server client. Is that right?

Also, I can see that hyper::Response is generic over Body, so maybe there is a way to use it with every structure which implements stream of Chunks? Or anything that implements AsRef<u8>. Anyway, even if this is not possible now, maybe we should just open an issue in hyper repo. Just to avoid unnecessary allocation, when read value is moved to a heap when creating a `Chunk.

abitrolly · 2017-10-01T05:46:36Z

There are a lot of warnings about .boxed() and BoxFuture in current builds. Making their removal a separate PR, like paritytech/jsonrpc#196 can speed up review.

tomusdrw · 2017-10-01T14:48:58Z

The warnings are most likely coming from jsonrpc libraries, that need to be bumped as well - and this refactoring is required for that.
I can split the PR, but I think there is not so many instances of .boxed() and BoxFuture in current codebase and it wouldn't really make this PR much smaller.

abitrolly · 2017-10-02T09:28:35Z

@tomusdrw exact list of libs with warnings, mined from this log - https://travis-ci.org/cyberFund/cybernode/builds/281880085#L1321

✗ ./warncounter.py
('hash', 1)
('node-health', 8)
('fetch', 5)
('patricia_trie', 169)
('parity-hash-fetch', 6)
('vm', 5)
('parity-whisper', 4)
('parity-local-store', 3)
('parity-rpc', 368)
('parity-rpc-client', 10)
('rpc-cli', 17)

warncounter.py

abitrolly · 2017-10-02T09:36:24Z

With versions:

✗ ./warncounter.py
    1  hash v0.1.0
    8  node-health v0.1.0
    5  fetch v0.1.0
  169  patricia_trie v0.1.0
    6  parity-hash-fetch v1.8.0
    5  vm v0.1.0
    4  parity-whisper v0.1.0
    3  parity-local-store v0.1.0
  368  parity-rpc v1.8.0
   10  parity-rpc-client v1.4.0
   17  rpc-cli v1.4.0

tomusdrw · 2017-10-02T13:26:36Z

So because most of the lines are in parity-rpc the PR wouldn't be smaller at all. Removing .boxed() from that part of code was done more elegantly because of the changes in jsonrpc-core (allowing to return T: IntoFuture instead of BoxFuture everywhere) that is being updated in this PR. So we can fix .boxed(), but this PR will change exactly the same lines to further remove it.

That said, I'm fine with doing that if @paritytech/core-devs believe it will help with getting this PR through.

gavofyork · 2017-10-03T08:13:21Z

rpc/src/v1/impls/eth.rs

 	}

 	fn block_by_number(&self, num: BlockNumber, include_txs: bool) -> BoxFuture<Option<RichBlock>, Error> {
-		future::done(self.block(num.into(), include_txs)).boxed()


why the change from .boxed() to Box::new?

boxed() has been deprecated. rust-lang/futures-rs#228

The motivation is that .boxed() requires Future + Send + 'static and sometimes it's not needed. Wrapping with Box::new covers both cases.

And usage of boxed causes warning during the compilation. It litters the build log significantly

tomusdrw · 2017-10-03T10:48:02Z

secret_store/src/key_server_cluster/cluster.rs

@@ -274,10 +274,6 @@ impl ClusterCore {
 		Box::new(net_connect(&node_address, handle, data.self_key_pair.clone(), disconnected_nodes)
 			.then(move |result| ClusterCore::process_connection_result(data, Some(node_address), result))
 			.then(|_| finished(())))
-<<<<<<< HEAD


Oh, thanks!

… td-update-jsonrpc

abitrolly · 2017-10-03T20:41:35Z

overall, looks fine, but the pr of that size is simply unreviewable

It is still +2,065 −2,881 with non-trivial changes mixed in.

tomusdrw added 11 commits September 5, 2017 12:14

Bump version.

850acca

Fix RPC crate.

b509414

Fix BoxFuture in crates.

fb7f0bf

Compiles and passes tests!

025f2da

Get rid of .boxed()

8a72209

Fixing issues with the UI.

c8f6e3d

Merge branch 'master' into td-update-jsonrpc

9095ec9

Remove minihttp. Support threads.

ccf0a18

Reimplement files serving to do it in chunks.

08ec476

Increase chunk size.

20f9908

Merge branch 'master' into td-update-jsonrpc

c072293

tomusdrw added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 15, 2017

tomusdrw added 5 commits September 15, 2017 14:15

Remove some unecessary copying.

3428ad1

Fix tests.

4d785ab

Fix stratum warning and ipfs todo.

648c8ec

Merge branch 'master' into td-update-jsonrpc

eda2846

Switch to proper branch of jsonrpc.

2e2373c

tomusdrw added A0-pleasereview 🤓 Pull request needs code review. and removed A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. labels Sep 15, 2017

tomusdrw added 2 commits September 18, 2017 09:22

Update Cargo.lock.

8fcafc6

Merge branch 'master' into td-update-jsonrpc

5bf85cd

gavofyork added ✓ certified and removed ✓ certified labels Sep 19, 2017

debris reviewed Sep 30, 2017

View reviewed changes

tomusdrw added 2 commits September 30, 2017 15:37

Merge branch 'master' into td-update-jsonrpc

a6a8e2b

Update docs.

c34bee9

Include dapps-glue in workspace.

5e04f45

tomusdrw mentioned this pull request Oct 2, 2017

Downgrade futures to suppress warnings. #6620

Merged

gavofyork reviewed Oct 3, 2017

View reviewed changes

tomusdrw and others added 2 commits October 3, 2017 12:21

Merge branch 'master' into td-update-jsonrpc

314898c

fixed merge artifacts

ecf26cb

tomusdrw commented Oct 3, 2017

View reviewed changes

tomusdrw added 2 commits October 3, 2017 21:56

Fix test compilation.

bbc7a4d

Merge branch 'td-update-jsonrpc' of github.com:paritytech/parity into…

7c78af6

… td-update-jsonrpc

arkpar added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 5, 2017

arkpar merged commit e8b418c into master Oct 5, 2017

arkpar deleted the td-update-jsonrpc branch October 5, 2017 10:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update jsonrpc dependencies and rewrite dapps to futures. #6522

Update jsonrpc dependencies and rewrite dapps to futures. #6522

tomusdrw commented Sep 15, 2017 •

edited

Loading

gavofyork commented Sep 18, 2017

debris left a comment

debris Sep 30, 2017

debris Sep 30, 2017

debris Sep 30, 2017

tomusdrw Sep 30, 2017 •

edited

Loading

debris Sep 30, 2017

debris Sep 30, 2017

abitrolly commented Oct 1, 2017 •

edited

Loading

tomusdrw commented Oct 1, 2017

abitrolly commented Oct 2, 2017 •

edited

Loading

abitrolly commented Oct 2, 2017

tomusdrw commented Oct 2, 2017

gavofyork Oct 3, 2017

debris Oct 3, 2017

tomusdrw Oct 3, 2017

grbIzl Oct 3, 2017

tomusdrw Oct 3, 2017

abitrolly commented Oct 3, 2017


		const MAX_CHUNK_SIZE: usize = 32 * 1024;

		pub struct Reader<R: io::Read> {

Update jsonrpc dependencies and rewrite dapps to futures. #6522

Update jsonrpc dependencies and rewrite dapps to futures. #6522

Conversation

tomusdrw commented Sep 15, 2017 • edited Loading

gavofyork commented Sep 18, 2017

debris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomusdrw Sep 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abitrolly commented Oct 1, 2017 • edited Loading

tomusdrw commented Oct 1, 2017

abitrolly commented Oct 2, 2017 • edited Loading

abitrolly commented Oct 2, 2017

tomusdrw commented Oct 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abitrolly commented Oct 3, 2017

tomusdrw commented Sep 15, 2017 •

edited

Loading

tomusdrw Sep 30, 2017 •

edited

Loading

abitrolly commented Oct 1, 2017 •

edited

Loading

abitrolly commented Oct 2, 2017 •

edited

Loading