Parity runtime moved to parity common for publication in crates.io#271
Conversation
* 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 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 * misc: remove_duplicate_empty_lines.sh * misc: run license header script * commit cargo lock
* 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.
|
It looks like @grbIzl hasn't signed our Contributor License Agreement, yet.
You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io Once you've signed, please reply to this thread with Many thanks, Parity Technologies CLA Bot |
| /// Executor for existing runtime. | ||
| /// | ||
| /// Deprecated: Exists only to connect with current JSONRPC implementation. | ||
| pub fn new(executor: TaskExecutor) -> Self { |
There was a problem hiding this comment.
perhaps add #[deprecated(note="some nice explaination"]
There was a problem hiding this comment.
I don't think it's used at all and I don't really understand what the comment (now deprecation descr) refers to. Would be good to get rid of this.
dvdplm
left a comment
dvdplm
left a comment
There was a problem hiding this comment.
The code looks good (added a few nitpicks) but I think the interesting part comes after this. Can you elaborate a little on how a freestanding app can use this library? Things I'd like to be able to do:
- move
secret-storeoutside of the main tree: I guess one would need to provide amain()and import this runtime + ethcore? - e.g. POA Networks has a HBBFT
Engineimplemented as a crate: how would they use it with parity-ethereum without including it in-tree? - maybe there's a way to resurrect
whisperin the same way assecret-storeif there's interest?
I guess what I'm asking about is an outline of what the path to re-usability looks like; I think this PR here is the first step of several? It would be nice to see a PoC of how it'd work though.
| enum Mode { | ||
| Tokio(TaskExecutor), | ||
| Sync, | ||
| ThreadPerFuture, |
There was a problem hiding this comment.
I wonder if these two are actually used outside tests?
There was a problem hiding this comment.
I didn't find any usages of this in production code (aka not tests).
There was a problem hiding this comment.
Let's add comments to clarify this.
| impl Executor { | ||
| /// Executor for existing runtime. | ||
| /// | ||
| /// Deprecated: Exists only to connect with current JSONRPC implementation. |
There was a problem hiding this comment.
Hmm. So this one is deprecated and new_sync and new_thread_per_future() is mostly used in tests? Doesn't add up!
There was a problem hiding this comment.
Everywhere in the code the executor is retrieved from runtime (it has executor() method for accessing it)
|
I put this PR onice for now, in order to sync it with SecretStore separation and prevent redundant changes after its publishing. |
| enum Mode { | ||
| Tokio(TaskExecutor), | ||
| Sync, | ||
| ThreadPerFuture, |
There was a problem hiding this comment.
Let's add comments to clarify this.
| } | ||
| } | ||
|
|
||
| /// Synchronous executor, used mostly for tests. |
There was a problem hiding this comment.
"mostly" or "only"? If it's tests-only, let's mark it #[cfg(test)].
|
@grbIzl status on this PR? You need to run |
As we agreed, I'm trying to separate SecretStore code first. And if it's feasible, proceed with this PR. PS I'm a little bit slow these days. |
|
I think adding at least one example to make sure api is exposed ok-ish is a must |
NikVolf
left a comment
NikVolf
left a comment
There was a problem hiding this comment.
Example missing for new crate
ordian
left a comment
ordian
left a comment
There was a problem hiding this comment.
I hope we can burn parity-runtime with fire someday.
|
looks good modulo @NikVolf's request |
|
As an example I've added test with simple read of the current dir, using runtime executor: https://github.com/paritytech/parity-common/pull/271/files#diff-11f21b7f145f98241270cf825e0aaab5R272 |
|
|
||
| /// Read current directory in future, which is executed in the created runtime | ||
| fn main() { | ||
| let fut = read_dir(".") |
There was a problem hiding this comment.
why not include this in doctest instead in the crate root to be part of the documentation?
There was a problem hiding this comment.
AFAIU the idea was to run this sample as standard cargo example in order to make sure, that public API is declared properly
| let runtime = Runtime::with_default_thread_count(); | ||
| runtime.executor().spawn(fut); | ||
| let timeout = Duration::from_secs(3); | ||
| park_timeout(timeout); |
There was a problem hiding this comment.
I wonder if this would help with openethereum/parity-ethereum#9636
| /// Executor for existing runtime. | ||
| /// | ||
| /// Deprecated: Exists only to connect with current JSONRPC implementation. | ||
| pub fn new(executor: TaskExecutor) -> Self { |
There was a problem hiding this comment.
I don't think it's used at all and I don't really understand what the comment (now deprecation descr) refers to. Would be good to get rid of this.
* master: prepare rlp-derive release (#344) Update/change licenses: MIT/Apache2.0 (#342) rlp-derive extracted (#343) Format for readme and changelog corrected (#341) Parity runtime moved to parity common for publication in crates.io (#271) Disable cache if explicit memory budget=0 passed (#339) [parity-crypto] prepare 0.5.0 (#336) [parity crypto]: remove unused depend `rustc_hex` (#337) Update doc comment (#335)
* master: kvdb-rocksdb: bump version (#348) kvdb-rocksdb: expose RocksDB stats (#347) Implement Error for FromDecStrErr (#346) Fix clippy lints for rlp-derive (#345) prepare rlp-derive release (#344) Update/change licenses: MIT/Apache2.0 (#342) rlp-derive extracted (#343) Format for readme and changelog corrected (#341) Parity runtime moved to parity common for publication in crates.io (#271) Disable cache if explicit memory budget=0 passed (#339)
8 participants
Current runtime wrapper is used in different components in Parity, but it's placed in parity-ethereum repository. That doesn't allow to use it outside Parity repository. After this move this dependency will be softened.
The following changes were made: