Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Whisper cli #8201

Merged
merged 13 commits into from Apr 9, 2018

Conversation

@niklasad1
Copy link
Member

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

This comment has been minimized.

Copy link

parity-cla-bot commented Mar 23, 2018

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

Many thanks,

Parity Technologies CLA Bot

@5chdn 5chdn added this to the 1.11 milestone Mar 23, 2018
@5chdn

This comment has been minimized.

Copy link
Contributor

5chdn commented Mar 23, 2018

Nice. /cc @gballet FYI

Needs docs once merged.

@niklasad1 niklasad1 force-pushed the whisper-cli branch from 75e61ec to 6619b24 Mar 23, 2018
@niklasad1 niklasad1 changed the title Whisper cli [more tests are needed therefore WIP] Whisper cli Mar 23, 2018
Options:
--whisper-pool-size SIZE Specify Whisper pool size [default: 10].
-p, --port PORT Specify which port to use [default: 8545].

This comment has been minimized.

Copy link
@rphmeier

rphmeier Apr 3, 2018

Member

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

@rphmeier

This comment has been minimized.

Copy link
Member

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 Apr 3, 2018
@niklasad1

This comment has been minimized.

Copy link
Member 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

This comment has been minimized.

Copy link
Member

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.

@niklasad1 niklasad1 force-pushed the whisper-cli branch from bf45f82 to d2ed942 Apr 4, 2018
@niklasad1 niklasad1 force-pushed the whisper-cli branch from d2ed942 to b690476 Apr 5, 2018
* Introduced an AtomicBool flag in FilterManager to cancel the `Decryption Worker Thread`
* Added some very basic test to faulty arguments
@niklasad1 niklasad1 force-pushed the whisper-cli branch from b690476 to a76eb9b Apr 5, 2018
@niklasad1

This comment has been minimized.

Copy link
Member Author

niklasad1 commented Apr 5, 2018

@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

This comment has been minimized.

Copy link
Member

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 🌨 and removed A1-onice 🌨 labels Apr 9, 2018
@debris
debris approved these changes Apr 9, 2018
@debris debris merged commit 1c75e8e into master Apr 9, 2018
1 check passed
1 check passed
continuous-integration/gitlab-test-rust-stable Build stage: test; status: success
Details
@debris debris deleted the whisper-cli branch Apr 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.