Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Whisper cli #8201

Merged
merged 13 commits into from
Apr 9, 2018
Merged

Whisper cli #8201

merged 13 commits into from
Apr 9, 2018

Conversation

niklasad1
Copy link
Collaborator

@niklasad1 niklasad1 commented Mar 23, 2018

Hey,

This is an attempt to close #6138

EDIT:
I removed the tests for because it more or less requires duplication of the existing tests in rpc and whisper because these are private I can't just use them directly.

It's alot of work re-implement (copy-paste) them so I don't think it is worth the effort. But maybe a couple of CLI arguments tests and some rough test that jsonrpc at least works!

Thoughts?

@parity-cla-bot
Copy link

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

Many thanks,

Parity Technologies CLA Bot

@5chdn 5chdn added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Mar 23, 2018
@5chdn 5chdn added this to the 1.11 milestone Mar 23, 2018
@5chdn
Copy link
Contributor

5chdn commented Mar 23, 2018

Nice. /cc @gballet FYI

Needs docs once merged.

@5chdn 5chdn added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Mar 23, 2018
@niklasad1 niklasad1 changed the title Whisper cli [more tests are needed therefore WIP] Whisper cli Mar 23, 2018
@niklasad1 niklasad1 added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Mar 23, 2018

Options:
--whisper-pool-size SIZE Specify Whisper pool size [default: 10].
-p, --port PORT Specify which port to use [default: 8545].
Copy link
Contributor

@rphmeier rphmeier Apr 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe clarify that this is the RPC port, not the devp2p port

@rphmeier
Copy link
Contributor

rphmeier commented Apr 3, 2018

except for the small grumble, LGTM.
w.r.t. tests I think tests that make sure the filter manager and other RPCs are working as expected would be nice (this might require a more abstraction around the pub/sub and network but maybe not much), but beyond that I don't see unit tests that make sure the RPCs actually invoke those methods as expected will be useful since the JSONRPC crate is well tested already. Those tests can be added in a follow-up PR.

@niklasad1 niklasad1 force-pushed the whisper-cli branch 2 times, most recently from 523a7c3 to bf45f82 Compare April 3, 2018 16:12
@niklasad1
Copy link
Collaborator Author

niklasad1 commented Apr 3, 2018

@rphmeier

I noticed a very interesting bug BTW if you run:

$ whisper-cli --port=24``` (i.e, a privileged port requires root access)

The error is not always propagated I guess it has wait for all other resources to released which may not happen that could be annoying for the user who may think that cli is running.

So I lean towards using expect instead of proper error handling but then the panic-hook will report this as a bug. Should I use expect and drop the panic-hook then?

@rphmeier
Copy link
Contributor

rphmeier commented Apr 4, 2018

I think we should use proper error handling; this failure is likely due to other bugs in the whisper, network, or RPC implementation and we should fix those rather than working around them.

* Introduced an AtomicBool flag in FilterManager to cancel the `Decryption Worker Thread`
* Added some very basic test to faulty arguments
@niklasad1
Copy link
Collaborator Author

@rphmeier

I think it is good to go now, but I had to introduce an additional variable to cancel/join the Decryption Worker Thread because the mspc sends Fn() closure which I can't determine whether it is an actual message or to cancel the thread.

Alternately we could just skip joining threads and terminate!

@rphmeier rphmeier added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Apr 5, 2018
@rphmeier
Copy link
Contributor

rphmeier commented Apr 5, 2018

Alternately we could just skip joining threads and terminate!

Yep, also an option. Either is an improvement over deadlock :)

@5chdn 5chdn added A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. and removed A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. labels Apr 9, 2018
@debris debris merged commit 1c75e8e into master Apr 9, 2018
@debris debris deleted the whisper-cli branch April 9, 2018 14:35
@5chdn 5chdn added the B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. label Apr 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Whisper CLI
5 participants