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

prevent silent errors in daemon mode #10007

Merged
merged 16 commits into from
Feb 1, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 14 additions & 10 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 5 additions & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ fake-fetch = { path = "util/fake-fetch" }
winapi = { version = "0.3.4", features = ["winsock2", "winuser", "shellapi"] }

[target.'cfg(not(windows))'.dependencies]
daemonize = "0.3"
parity-daemonize = "0.1.1"

[features]
miner-debug = ["ethcore/miner-debug"]
Expand Down Expand Up @@ -133,6 +133,10 @@ members = [
"evmbin",
"parity-clib",
"whisper/cli",
"util/triehash-ethereum",
"util/keccak-hasher",
"util/patricia-trie-ethereum",
"util/fastmap"
]

[patch.crates-io]
Expand Down
6 changes: 3 additions & 3 deletions parity-clib/Parity.java
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,9 @@ public class Parity {
*
* @param options The CLI options to start Parity with
*/
public Parity(String[] options) {
public Parity(String[] options, String loggerMode, String loggerFile) {
long config = configFromCli(options);
inner = build(config);
inner = build(config, loggerMode, loggerFile);
}

/** Performs an asynchronous RPC query by spawning a background thread that is executed until
Expand Down Expand Up @@ -76,7 +76,7 @@ public void unsubscribeWebSocket(long session) {
}

private static native long configFromCli(String[] cliOptions);
private static native long build(long config);
private static native long build(long config, String loggerMode, String loggerFile);
private static native void destroy(long inner);
private static native void rpcQueryNative(long inner, String rpc, long timeoutMillis, Object callback);
private static native long subscribeWebSocketNative(long inner, String rpc, Object callback);
Expand Down
9 changes: 6 additions & 3 deletions parity-clib/examples/cpp/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,8 @@ const std::vector<std::string> ws_subscriptions {
void callback(void* user_data, const char* response, size_t _len) {
Callback* cb = static_cast<Callback*>(user_data);
if (cb->type == CALLBACK_RPC) {
printf("rpc response: %s\r\n", response);
cb->counter -= 1;
} else if (cb->type == CALLBACK_WS) {
printf("websocket response: %s\r\n", response);
std::regex is_subscription ("\\{\"jsonrpc\":\"2.0\",\"result\":\"0[xX][a-fA-F0-9]{16}\",\"id\":1\\}");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Removed these printouts doesn't make sense anymore when these can be controlled via the logger

if (std::regex_match(response, is_subscription) == true) {
cb->counter -= 1;
Expand Down Expand Up @@ -153,7 +151,8 @@ void* parity_run(std::vector<const char*> args) {
ParityParams cfg = {
.configuration = nullptr,
.on_client_restart_cb = callback,
.on_client_restart_cb_custom = nullptr
.on_client_restart_cb_custom = nullptr,
.logger = nullptr
};

std::vector<size_t> str_lens;
Expand All @@ -173,6 +172,10 @@ void* parity_run(std::vector<const char*> args) {
}
}

// enable logging but only the `rpc module` and don't write it to a file
char log_mode [] = "rpc=trace";
parity_set_logger(log_mode, strlen(log_mode), nullptr, 0, &cfg.logger);

void *parity = nullptr;
if (parity_start(&cfg, &parity) != 0) {
return nullptr;
Expand Down
10 changes: 3 additions & 7 deletions parity-clib/examples/java/Main.java
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,9 @@ class Main {
};

public static void runParity(String[] config) {
Parity parity = new Parity(config);
String loggerMode = "rpc=trace";
String loggerFile = "foo.log";
Parity parity = new Parity(config, loggerMode, loggerFile);

Callback rpcCallback = new Callback(1);
Callback webSocketCallback = new Callback(2);
Expand Down Expand Up @@ -94,12 +96,6 @@ public Callback(int type) {
}

public void callback(Object response) {
response = (String) response;
if (callbackType == 1) {
System.out.println("rpc: " + response);
} else if (callbackType == 2) {
System.out.println("ws: " + response);
}
counter.getAndIncrement();
}

Expand Down
30 changes: 30 additions & 0 deletions parity-clib/parity.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ struct ParityParams {

/// Custom parameter passed to the `on_client_restart_cb` callback as first parameter.
void *on_client_restart_cb_custom;

/// Logger object which must be created by the `parity_config_logger` function
void *logger;
};

#ifdef __cplusplus
Expand Down Expand Up @@ -63,6 +66,33 @@ extern "C" {
///
int parity_config_from_cli(char const* const* args, size_t const* arg_lens, size_t len, void** out);

/// Builds a new logger object which should be a member of the `ParityParams struct`
///
/// - log_mode : String representing the log mode according to `Rust LOG` or nullptr to disable logging.
/// See module documentation for `ethcore-logger` for more info.
/// - log_mode_len : Length of the log_mode or zero to disable logging
/// - log_file : String respresenting the file name to write to log to or nullptr to disable logging to a file
/// - log_mode_len : Length of the log_file or zero to disable logging to a file
/// - logger : Pointer to point to the created `Logger` object

/// **Important**: This function must only be called exactly once otherwise it will panic. If you want to disable a
/// logging mode or logging to a file make sure that you pass the `length` as zero
///
/// # Example
///
/// ```no_run
/// void* cfg;
/// const char *args[] = {"--light", "--can-restart"};
/// size_t str_lens[] = {7, 13};
/// if (parity_config_from_cli(args, str_lens, 2, &cfg) != 0) {
/// return 1;
/// }
/// char[] logger_mode = "rpc=trace";
/// parity_set_logger(logger_mode, strlen(logger_mode), nullptr, 0, &cfg.logger);
/// ```
///
int parity_set_logger(const char* log_mode, size_t log_mode_len, const char* log_file, size_t log_file_len, void** logger);

/// Destroys a configuration object created earlier.
///
/// **Important**: You probably don't need to call this function. Calling `parity_start` destroys
Expand Down
18 changes: 15 additions & 3 deletions parity-clib/src/java.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use std::time::Duration;
use std::thread;
use std::os::raw::c_void;

use {parity_config_from_cli, parity_destroy, parity_start, parity_unsubscribe_ws, ParityParams, error};
use {parity_config_from_cli, parity_destroy, parity_set_logger, parity_start, parity_unsubscribe_ws, ParityParams, error};

use futures::{Future, Stream};
use futures::sync::mpsc;
Expand Down Expand Up @@ -96,12 +96,24 @@ pub unsafe extern "system" fn Java_io_parity_ethereum_Parity_configFromCli(env:
}

#[no_mangle]
pub unsafe extern "system" fn Java_io_parity_ethereum_Parity_build(env: JNIEnv, _: JClass, config: va_list) -> jlong {
let params = ParityParams {
pub unsafe extern "system" fn Java_io_parity_ethereum_Parity_build(
env: JNIEnv,
_: JClass,
config: va_list,
logger_mode: JString,
logger_file: JString
) -> jlong {
let mut params = ParityParams {
configuration: config,
.. mem::zeroed()
};

let logger_mode: String = env.get_string(logger_mode).expect("valid JString; qed").into();
let logger_file: String = env.get_string(logger_file).expect("valid JString; qed").into();

parity_set_logger(logger_mode.as_ptr(), logger_mode.as_bytes().len(), logger_file.as_ptr(),
logger_file.as_bytes().len(), &mut params.logger);

let mut out = ptr::null_mut();
match parity_start(&params, &mut out) {
0 => out as jlong,
Expand Down
23 changes: 22 additions & 1 deletion parity-clib/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ pub struct ParityParams {
pub configuration: *mut c_void,
pub on_client_restart_cb: Callback,
pub on_client_restart_cb_custom: *mut c_void,
pub logger: *mut c_void
}

#[no_mangle]
Expand Down Expand Up @@ -112,6 +113,7 @@ pub unsafe extern fn parity_start(cfg: *const ParityParams, output: *mut *mut c_
*output = ptr::null_mut();
let cfg: &ParityParams = &*cfg;

let logger = Arc::from_raw(cfg.logger as *mut parity_ethereum::RotatingLogger);
let config = Box::from_raw(cfg.configuration as *mut parity_ethereum::Configuration);

let on_client_restart_cb = {
Expand All @@ -122,7 +124,7 @@ pub unsafe extern fn parity_start(cfg: *const ParityParams, output: *mut *mut c_
move |new_chain: String| { cb.call(new_chain.as_bytes()); }
};

let action = match parity_ethereum::start(*config, on_client_restart_cb, || {}) {
let action = match parity_ethereum::start(*config, logger, on_client_restart_cb, || {}) {
Ok(action) => action,
Err(_) => return 1,
};
Expand Down Expand Up @@ -262,6 +264,25 @@ pub unsafe extern fn parity_set_panic_hook(callback: Callback, param: *mut c_voi
});
}

#[no_mangle]
pub unsafe extern fn parity_set_logger(
logger_mode: *const u8,
logger_mode_len: usize,
log_file: *const u8,
log_file_len: usize,
logger: *mut *mut c_void) {

let mut logger_cfg = parity_ethereum::LoggerConfig::default();
logger_cfg.mode = String::from_utf8(slice::from_raw_parts(logger_mode, logger_mode_len).to_owned()).ok();

// Make sure an empty string is not constructed as file name (to prevent panic)
if log_file_len != 0 && !log_file.is_null() {
logger_cfg.file = String::from_utf8(slice::from_raw_parts(log_file, log_file_len).to_owned()).ok();
}

*logger = Arc::into_raw(parity_ethereum::setup_log(&logger_cfg).expect("Logger initialized only once; qed")) as *mut _;
}

// Internal structure for handling callbacks that get passed a string.
struct CallbackStr {
user_data: *mut c_void,
Expand Down
3 changes: 2 additions & 1 deletion parity/configuration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,8 @@ impl Configuration {
}
}

fn logger_config(&self) -> LogConfig {
/// returns logger config
pub fn logger_config(&self) -> LogConfig {
LogConfig {
mode: self.args.arg_logging.clone(),
color: !self.args.flag_no_color && !cfg!(windows),
Expand Down
33 changes: 21 additions & 12 deletions parity/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,19 +111,22 @@ mod user_defaults;
mod whisper;
mod db;

use std::io::BufReader;
use std::fs::File;
use hash::keccak_buffer;
use std::io::BufReader;
use std::sync::Arc;

use cli::Args;
use configuration::{Cmd, Execute};
use deprecated::find_deprecated;
use ethcore_logger::setup_log;
use hash::keccak_buffer;

#[cfg(feature = "memory_profiling")]
use std::alloc::System;

pub use self::configuration::Configuration;
pub use self::run::RunningClient;
pub use parity_rpc::PubSubSession;
pub use ethcore_logger::{Config as LoggerConfig, setup_log, RotatingLogger};

#[cfg(feature = "memory_profiling")]
#[global_allocator]
Expand Down Expand Up @@ -180,14 +183,13 @@ pub enum ExecutionAction {
Running(RunningClient),
}

fn execute<Cr, Rr>(command: Execute, on_client_rq: Cr, on_updater_rq: Rr) -> Result<ExecutionAction, String>
fn execute<Cr, Rr>(
command: Execute,
logger: Arc<RotatingLogger>,
on_client_rq: Cr, on_updater_rq: Rr) -> Result<ExecutionAction, String>
where Cr: Fn(String) + 'static + Send,
Rr: Fn() + 'static + Send
{
// TODO: move this to `main()` and expose in the C API so that users can setup logging the way
// they want
let logger = setup_log(&command.logger).expect("Logger is initialized only once; qed");

#[cfg(feature = "deadlock_detection")]
run_deadlock_detection_thread();

Expand Down Expand Up @@ -221,14 +223,21 @@ fn execute<Cr, Rr>(command: Execute, on_client_rq: Cr, on_updater_rq: Rr) -> Res
/// binary.
///
/// On error, returns what to print on stderr.
pub fn start<Cr, Rr>(conf: Configuration, on_client_rq: Cr, on_updater_rq: Rr) -> Result<ExecutionAction, String>
where Cr: Fn(String) + 'static + Send,
Rr: Fn() + 'static + Send
// FIXME: totally independent logging capability, see https://github.com/paritytech/parity-ethereum/issues/10252
pub fn start<Cr, Rr>(
conf: Configuration,
logger: Arc<RotatingLogger>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@tomaka tomaka Dec 27, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

on_client_rq: Cr,
on_updater_rq: Rr
) -> Result<ExecutionAction, String>
where
Cr: Fn(String) + 'static + Send,
Rr: Fn() + 'static + Send
{
let deprecated = find_deprecated(&conf.args);
for d in deprecated {
println!("{}", d);
}

execute(conf.into_command()?, on_client_rq, on_updater_rq)
execute(conf.into_command()?, logger, on_client_rq, on_updater_rq)
}
2 changes: 1 addition & 1 deletion parity/logger/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ pub fn setup_log(config: &Config) -> Result<Arc<RotatingLogger>, String> {
let maybe_file = match config.file.as_ref() {
Some(f) => Some(open_options
.append(true).create(true).open(f)
.map_err(|_| format!("Cannot write to log file given: {}", f))?),
.map_err(|e| format!("Cannot write to log file given: {}, {}", f, e))?),
None => None,
};

Expand Down
Loading