prevent silent errors in daemon mode by seunlanlege · Pull Request #10007 · openethereum/parity-ethereum

seunlanlege · 2018-12-03T15:42:53Z

This PR replaces daemonize from crates.io, with a custom daemonize

The custom daemonize pipes the daemon's STDOUT/STDERR to the parent process, after parity-ethereum successfully starts the daemon process detaches from the parent and the parent exits.

TODO:

Update Java bindings

parity-cla-bot · 2018-12-03T15:42:57Z

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

Many thanks,

Parity Technologies CLA Bot

ordian

It fails silently on my linux laptop (need to investigate further).
Open questions:

Do we want to maintain our custom fork (or rather reimplementation) of daemonize?
If we do, should it be placed in util/daemonize? (E.g. why not in a separate repo)

niklasad1 · 2018-12-04T15:20:35Z

If we want to maintain this it should go in a separate repo preferable forked under the parity org as we have done with other forks such as ring, rocksdb etc

seunlanlege · 2018-12-05T13:37:17Z

not sure why the c++ example isn't compiling @5chdn

ordian

The c++ example is failing because the interface for start has changed, clib needs to be updated here.

The code looks good in general, but I still think we should move it to a separate repository (and maybe publish to crates.io), because it seems like a general improvement (modulo missing features) over daemonize, which others would probably want to use (and it seems like daemonize is unmaintained atm). Also we need tests like these and it would be nice to run them on linux and macOS.

niklasad1 · 2018-12-05T15:08:37Z

-pub fn start<Cr, Rr>(conf: Configuration, on_client_rq: Cr, on_updater_rq: Rr) -> Result<ExecutionAction, String>
+pub fn start<Cr, Rr>(
+	conf: Configuration,
+	logger: Arc<RotatingLogger>,


This breaks the C-bindings in parity-clib you need to update them accordingly

The difficult part of #8574 (and the reason why it wasn't done immediately when parity-clib was created) is that ideally the RotatingLogger wouldn't be exposed in the API.

However I guess we can ignore the problem in the name of "meh, who cares", and open another issue for that specific thing instead.

We're not exposing RotatingLogger in C API, we expose only Logger params struct.

I mentioned "when parity-clib was created", because the Rust lib and the C library were created at the same time.

The logging system is unfortunately a global variable, and thus it shouldn't be the job of a library to set it.
The consequence is that if a program wants to use parity-as-a-library, they cannot setup their own logging compatible with the log crate.
An ideal world, it is main.rs that would setup the logging and that's it. The C library would have an independent function (not tied to parity, just like parity_set_panic_hook is totally independent as well) that sets up the Rust logging and explicitly mentions in its documentation that this applies to the log Rust crate as a whole.

niklasad1 · 2018-12-05T15:13:58Z

@seunlanlege The changes you did parity::start breaks the C bindings and you need to add a logger object there both in parity.h and in main.cpp. I can help you if you are not familiar with C/C++

seunlanlege · 2018-12-05T15:51:41Z

@niklasad1 please do, thanks.

seunlanlege · 2018-12-07T16:47:37Z

moved the implementation to here

ordian · 2018-12-10T10:37:30Z

FYI: The naming convention for rust crates from the api-guidelines:

Crate names should not use -rs or -rust as a suffix or prefix. Every crate is Rust! It serves no purpose to remind users of this constantly.

Could you remove util/daemonize, also clib logger API needs to be sorted out.

niklasad1 · 2018-12-12T14:28:37Z

 ///
 /// Returns 0 on success, and non-zero on error.
-int parity_start(const ParityParams* params, void** out);
+int parity_start(const ParityParams* params, Logger logger, void** out);


Well, we could also pass Logger as a pointer to be uniform but I decided to pass it by value to reduce the possibility for deref NULL in the library. However, it will requre 4 words of memory instead of 1 word and it will probably not fit in the registers and need to pushed on the stack (assuming not inlined)

It is still possible to deref NULL if a null pointer is passed as string with len != 0

niklasad1 · 2018-12-12T15:46:15Z

 		let cfg: &ParityParams = &*cfg;

+		let mode = {
+			if logger.mode_len == 0 {


Make sure an empty string is not constructed!

We're not using this check in parity_config_from_cli, I think it's safe since mem::align_of::<u8> == 1 and also String::from_utf8(slice.to_owned()) does not allocate if the slice.is_empty().

Yeah, it is redundant thus it should be removed

However, when it comes to log-file we need the check because an empty string as file name will be a run-time error/panic

niklasad1 · 2018-12-12T15:46:38Z

+		};
+
+		let file = {
+			if logger.file_len == 0 {


Make sure an empty string is not constructed!

seunlanlege · 2018-12-18T22:07:57Z

@5chdn I can't sign in to gitlab, could you please restart the android build?

niklasad1 · 2018-12-19T11:52:39Z

@seunlanlege I restarted it but it looks like you need to bump parity-daemonize to get the latest changes

seunlanlege · 2018-12-20T13:27:01Z

looks like alls good here

ordian

LGTM.

ordian · 2018-12-20T14:39:10Z

 		let cfg: &ParityParams = &*cfg;

+		let mode = {
+			if logger.mode_len == 0 {


We're not using this check in parity_config_from_cli, I think it's safe since mem::align_of::<u8> == 1 and also String::from_utf8(slice.to_owned()) does not allocate if the slice.is_empty().

tomaka · 2019-01-28T13:15:28Z

I'm fine with leaving it as it is, because doing it properly requires a lot of work.
I opened #10252.

…ting to pid file

…g to create logfile

Co-Authored-By: seunlanlege <seunlanlege@gmail.com>

@tomaka

* Add FIXME comment regarding @tomaka grumbles * Unify logger with the C-API in ParityParams (less type-safety with more from_raw() conversions) * Add better documentation in the `parity.h`

Co-Authored-By: seunlanlege <seunlanlege@gmail.com>

* Update example to the API changes * Remove needless printouts which can be controlled via logger instead

* master: Fix Windows build (#10284) Don't run the CPP example on CI (#10285) Additional tests for uint deserialization. (#10279) prevent silent errors in daemon mode (#10007)

seunlanlege requested review from dvdplm, ordian and tomaka and removed request for dvdplm and tomaka December 3, 2018 15:44

seunlanlege added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Dec 3, 2018

ordian reviewed Dec 4, 2018

View reviewed changes

Comment thread parity/main.rs Outdated

Comment thread parity/main.rs Outdated

Comment thread util/daemonize/src/lib.rs Outdated

Comment thread util/daemonize/src/lib.rs Outdated

Comment thread util/daemonize/src/lib.rs Outdated

Comment thread util/daemonize/src/lib.rs Outdated

5chdn added this to the 2.3 milestone Dec 4, 2018

ordian reviewed Dec 5, 2018

View reviewed changes

Comment thread parity/main.rs Outdated

Comment thread parity/main.rs Outdated

Comment thread parity/lib.rs Outdated

Comment thread util/daemonize/src/lib.rs Outdated

Comment thread util/daemonize/src/lib.rs Outdated

Comment thread Cargo.toml Outdated

niklasad1 reviewed Dec 5, 2018

View reviewed changes

niklasad1 force-pushed the daemonize branch 3 times, most recently from 8a0125e to df07353 Compare December 11, 2018 21:47

niklasad1 reviewed Dec 12, 2018

View reviewed changes

ordian approved these changes Dec 20, 2018

View reviewed changes

tomaka approved these changes Jan 28, 2019

View reviewed changes

niklasad1 force-pushed the daemonize branch from 7218e37 to e9a52f1 Compare January 29, 2019 09:12

seunlanlege and others added 16 commits January 30, 2019 19:04

prevent silent errors in daemon mode

d53b9e9

change author in Cargo.toml, add preamble to pipe.rs

37fd2c8

set the uid and gid on daemon process, fix permission errors when wri…

c8c71d9

…ting to pid file

call setup_logger before daemonize to prevent crashing when attemptin…

392dafb

…g to create logfile

map_err for calls to splice and ioctl, fix spaces in Cargo.toml

6311b2c

split out daemonize to own repo

1bde838

removed util/daemonize

4291552

renamed dep to parity-daemonize

8a7e262

fix(parity-clib): enable logger

def13ad

bump parity-daemonize

6245d50

remove obsolete comment

bbdaf66

Co-Authored-By: seunlanlege <seunlanlege@gmail.com>

fix(grumbles): docs and log in ParityParams

a0752e1

* Add FIXME comment regarding @tomaka grumbles * Unify logger with the C-API in ParityParams (less type-safety with more from_raw() conversions) * Add better documentation in the `parity.h`

Apply suggestions from code review

574805c

Co-Authored-By: seunlanlege <seunlanlege@gmail.com>

docs(parity lib): add link to logging issue

ef856ef

fix(parity-clib): JNI enable logger

1efbb72

fix(parity-clib): update Java example

c93e31c

* Update example to the API changes * Remove needless printouts which can be controlled via logger instead

niklasad1 force-pushed the daemonize branch from e9a52f1 to c93e31c Compare January 30, 2019 18:04

5chdn merged commit 0f9b221 into master Feb 1, 2019

5chdn deleted the daemonize branch February 1, 2019 18:31

This was referenced Feb 3, 2019

Fix Windows build #10284

Merged

Don't run the CPP example on CI #10285

Merged

ordian added a commit that referenced this pull request Feb 3, 2019

Merge branch 'master' into ring-0.14

bcdcac2

* master: Fix Windows build (#10284) Don't run the CPP example on CI (#10285) Additional tests for uint deserialization. (#10279) prevent silent errors in daemon mode (#10007)

HCastano mentioned this pull request Feb 27, 2019

More Backports for Stable 2.3.5 #10430

Merged

ordian added a commit that referenced this pull request Apr 5, 2019

Merge branch 'master' into ao-lmdb

fb06bd6

* master: Fix Windows build (#10284) Don't run the CPP example on CI (#10285) Additional tests for uint deserialization. (#10279) prevent silent errors in daemon mode (#10007)

Conversation

seunlanlege commented Dec 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

parity-cla-bot commented Dec 3, 2018

Uh oh!

ordian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

niklasad1 commented Dec 4, 2018

Uh oh!

seunlanlege commented Dec 5, 2018

Uh oh!

ordian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

niklasad1 Dec 5, 2018

Choose a reason for hiding this comment

Uh oh!

tomaka Dec 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ordian Dec 27, 2018

Choose a reason for hiding this comment

Uh oh!

tomaka Dec 27, 2018

Choose a reason for hiding this comment

Uh oh!

niklasad1 commented Dec 5, 2018

Uh oh!

seunlanlege commented Dec 5, 2018

Uh oh!

seunlanlege commented Dec 7, 2018

Uh oh!

ordian commented Dec 10, 2018

Uh oh!

niklasad1 Dec 12, 2018

Choose a reason for hiding this comment

Uh oh!

niklasad1 Dec 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ordian Dec 20, 2018

Choose a reason for hiding this comment

Uh oh!

niklasad1 Jan 3, 2019

Choose a reason for hiding this comment

Uh oh!

niklasad1 Dec 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seunlanlege commented Dec 18, 2018

Uh oh!

niklasad1 commented Dec 19, 2018

Uh oh!

seunlanlege commented Dec 20, 2018

Uh oh!

ordian left a comment

Choose a reason for hiding this comment

Uh oh!

ordian Dec 20, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tomaka commented Jan 28, 2019

seunlanlege commented Dec 3, 2018 •

edited

Loading

tomaka Dec 27, 2018 •

edited

Loading

niklasad1 Dec 12, 2018 •

edited

Loading

niklasad1 Dec 12, 2018 •

edited

Loading