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

Insert explicit warning into the panic hook #11225

Merged
merged 1 commit into from
Oct 31, 2019
Merged

Insert explicit warning into the panic hook #11225

merged 1 commit into from
Oct 31, 2019

Conversation

grbIzl
Copy link
Collaborator

@grbIzl grbIzl commented Oct 31, 2019

This additional warning should help to debug the use case, when stderr is setup to put output somewhere else (not in Parity log).
In particular, I had this issue, when Parity was running as daemon and one of its threads panicked. The log looked very innocent, no panic was shown and it was not easy to understand the reason, why Parity quits.

@grbIzl grbIzl requested review from dvdplm and ordian October 31, 2019 10:15
@grbIzl grbIzl added A0-pleasereview 🤓 Pull request needs code review. F3-annoyance 💩 The client behaves within expectations, however this “expected behaviour” itself is at issue. labels Oct 31, 2019
ordian
ordian approved these changes Oct 31, 2019
View reviewed changes
Copy link
Collaborator

@ordian ordian left a comment

Choose a reason for hiding this comment

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

I would still suggest moving eprintln! to a warn! below as stderr is not redirected to a parity log when running it as a (systemd-) daemon.

@ordian ordian added this to the 2.7 milestone Oct 31, 2019
@ordian ordian added B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. B1-patch-beta 🕷🕷 M4-core ⛓ Core client code / Rust. and removed F3-annoyance 💩 The client behaves within expectations, however this “expected behaviour” itself is at issue. labels Oct 31, 2019
@dvdplm
Copy link
Collaborator

dvdplm commented Oct 31, 2019

Shouldn't the daemonizer script be responsible for setting up stdout/stderr? How did you run parity on your end? parity daemon? Or…?

@grbIzl
Copy link
Collaborator Author

grbIzl commented Oct 31, 2019

Shouldn't the daemonizer script be responsible for setting up stdout/stderr? How did you run parity on your end? parity daemon? Or…?

The script does setup this. But I was not looking for the panic as there were no any signs of it in the logs. We considered all other things, that can cause such behavior (OOM, misconfig, cache-size etc). And it was not my setup btw, so it also made things harder.

@grbIzl
Copy link
Collaborator Author

grbIzl commented Oct 31, 2019

I would still suggest moving eprintln! to a warn! below as stderr is not redirected to a parity log when running it as a (systemd-) daemon.

I just don't want to break the current behavior, as something can depend on it.

dvdplm
dvdplm approved these changes Oct 31, 2019
View reviewed changes
Copy link
Collaborator

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

TBH I don't know what the best practice is for daemonizing and stdout/stderr, but I know there's more to it than first meets the eye; this seems a little bit fishy to me.
However, if you're convinced it's useful/needed for some setups, then I'm fine merging this. :)

@grbIzl grbIzl merged commit 80754c3 into master Oct 31, 2019
@grbIzl grbIzl deleted the ShowExplicitPanicWarn branch October 31, 2019 15:17
This was referenced Nov 5, 2019
Merged
Merged
niklasad1 pushed a commit that referenced this pull request Nov 5, 2019
@grbIzl @niklasad1
Insert explicit warning into the panic hook (#11225)
e612977
dvdplm added a commit that referenced this pull request Nov 6, 2019
@dvdplm
Merge branch 'master' into dp/chore/add-bench-for-to_fat_rlps
89cdb3b 
* master: (21 commits)
  ropsten #6631425 foundation #8798209 (#11201)
  Update list of bootnodes for xDai chain (#11236)
  ethcore/res: add mordor testnet configuration (#11200)
  [chain specs]: activate `Istanbul` on mainnet (#11228)
  [builtin]: support `multiple prices and activations` in chain spec (#11039)
  Insert explicit warning into the panic hook (#11225)
  Snapshot restoration overhaul (#11219)
  Fix docker centos build (#11226)
  retry on gitlab system failures (#11222)
  Update bootnodes. (#11203)
  Use provided usd-per-eth value if an endpoint is specified (#11209)
  Use a lock instead of atomics for snapshot Progress (#11197)
  [informant]: `MillisecondDuration` -> `as_millis()` (#11211)
  Step duration map configuration parameter ported from the POA Network fork (#10902)
  Upgrade jsonrpc to latest (#11206)
  [export hardcoded sync]: use debug for `H256` (#11204)
  Pause pruning while snapshotting (#11178)
  Type annotation for next_key() matching of json filter options (#11192)
  Crypto primitives removed from ethkey (#11174)
  Made ecrecover implementation trait public (#11188)
  ...
s3krit pushed a commit that referenced this pull request Nov 6, 2019
@grbIzl @s3krit
Insert explicit warning into the panic hook (#11225)
5470802
dvdplm added a commit that referenced this pull request Nov 6, 2019
@dvdplm
Merge branch 'v2.5.10-stable' of github.com:paritytech/parity-ethereu…
8efee67 
…m into v2.5.10-stable

* 'v2.5.10-stable' of github.com:paritytech/parity-ethereum:
  Update bootnodes. (#11203)
  Insert explicit warning into the panic hook (#11225)
dvdplm pushed a commit that referenced this pull request Nov 7, 2019
@grbIzl @dvdplm
Insert explicit warning into the panic hook (#11225)
9a8962e
s3krit pushed a commit that referenced this pull request Nov 11, 2019
@soc1c @s3krit
v2.5.10 stable (#11239)
f3cdd7b 
* ropsten #6631425 foundation #8798209 (#11201)
* [stable] builtin, istanbul and mordor testnet backports (#11234)
  * ethcore-builtin (#10850)
  * [builtin]: support `multiple prices and activations` in chain spec (#11039)
  * [chain specs]: activate `Istanbul` on mainnet (#11228)
  * ethcore/res: add mordor testnet configuration (#11200)
* Update list of bootnodes for xDai chain (#11236)
* ethcore: remove `test-helper feat` from build (#11047)
* Secret store: fix Instant::now() related race in net_keep_alive (#11155) (#11159)
* [stable]: backport #10691 and #10683 (#11143)
  * Fix compiler warning (that will become an error) (#10683)
  * Refactor Clique stepping (#10691)
* Add Constantinople eips to the dev (instant_seal) config (#10809)
* Add cargo-remote dir to .gitignore (?)
* Insert explicit warning into the panic hook (#11225)
* Fix docker centos build (#11226)
* Update MIX bootnodes. (#11203)
* Use provided usd-per-eth value if an endpoint is specified (#11209)
* Add new line after writing block to hex file. (#10984)
* Type annotation for next_key() matching of json filter options (#11192) (but no `FilterOption` in 2.5 so…)
* Upgrade jsonrpc to latest (#11206)
* [CI] check evmbin build (#11096)
* Correct EIP-712 encoding (#11092)
* [client]: Fix for incorrectly dropped consensus messages (#11086)
* Fix block detail updating (#11015)
* Switching sccache from local to Redis (#10971)
* Made ecrecover implementation trait public (#11188)
* [dependencies]: jsonrpc `14.0.1` (#11183)
* [receipt]: add `sender` & `receiver` to `RichReceipts` (#11179)
* [ethcore/builtin]: do not panic in blake2pricer on short input (#11180)
* util Host: fix a double Read Lock bug in fn Host::session_readable() (#11175)
* ethcore client: fix a double Read Lock bug in fn Client::logs() (#11172)
* Change how RPCs eth_call and eth_estimateGas handle "Pending" (#11127)
* Cleanup stratum a bit (#11161)
* Upgrade to jsonrpc v14 (#11151)
* SecretStore: expose restore_key_public in HTTP API (#10241)
s3krit pushed a commit that referenced this pull request Nov 11, 2019
@soc1c @s3krit
v2.6.5 beta (#11240)
bc90ba2 
* [CI] check evmbin build (#11096)
* Correct EIP-712 encoding (#11092)
* [client]: Fix for incorrectly dropped consensus messages (#11082) (#11086)
* Update hardcoded headers (foundation, classic, kovan, xdai, ewc, ...) (#11053)
* Add cargo-remote dir to .gitignore (?)
* Update light client headers: ropsten 6631425 foundation 8798209 (#11201)
* Update list of bootnodes for xDai chain (#11236)
* ethcore/res: add mordor testnet configuration (#11200)
* [chain specs]: activate Istanbul on mainnet (#11228)
* [builtin]: support multiple prices and activations in chain spec (#11039)
* [receipt]: add sender & receiver to RichReceipts (#11179)
* [ethcore/builtin]: do not panic in blake2pricer on short input (#11180)
* Made ecrecover implementation trait public (#11188)
* Fix docker centos build (#11226)
* Update MIX bootnodes. (#11203)
* Insert explicit warning into the panic hook (#11225)
* Use provided usd-per-eth value if an endpoint is specified (#11209)
* Cleanup stratum a bit (#11161)
* Add Constantinople EIPs to the dev (instant_seal) config (#10809) (already backported)
* util Host: fix a double Read Lock bug in fn Host::session_readable() (#11175)
* ethcore client: fix a double Read Lock bug in fn Client::logs() (#11172)
* Type annotation for next_key() matching of json filter options (#11192)
* Upgrade jsonrpc to latest (#11206)
* [dependencies]: jsonrpc 14.0.1 (#11183)
* Upgrade to jsonrpc v14 (#11151)
* Switching sccache from local to Redis (#10971)
* Snapshot restoration overhaul (#11219)
* Add new line after writing block to hex file. (#10984)
* Pause pruning while snapshotting (#11178)
* Change how RPCs eth_call and eth_estimateGas handle "Pending" (#11127)
* Fix block detail updating (#11015)
* Make InstantSeal Instant again #11186
* Filter out some bad ropsten warp snapshots (#11247)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ordian ordian ordian approved these changes

@dvdplm dvdplm dvdplm approved these changes

Assignees
No one assigned
Labels
A0-pleasereview 🤓 Pull request needs code review. B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. M4-core ⛓ Core client code / Rust.
Projects
None yet
Milestone
2.7
Development

Successfully merging this pull request may close these issues.
3 participants
@grbIzl @dvdplm @ordian