From 27a672e66c43f40b2f633f5a735e5bd7bf132796 Mon Sep 17 00:00:00 2001 From: DaniPopes <57450786+DaniPopes@users.noreply.github.com> Date: Tue, 11 Jun 2024 08:35:31 +0200 Subject: [PATCH 1/3] chore: remove direct tracing calls from proc-macros --- core/src/lib.rs | 5 ++ core/src/proc_macros_support.rs | 10 +++ proc-macros/src/render_server.rs | 120 ++++++++++--------------------- 3 files changed, 53 insertions(+), 82 deletions(-) create mode 100644 core/src/proc_macros_support.rs diff --git a/core/src/lib.rs b/core/src/lib.rs index e23bc54006..9b69010f9f 100644 --- a/core/src/lib.rs +++ b/core/src/lib.rs @@ -68,6 +68,9 @@ pub type RpcResult = std::result::Result; +#[doc(hidden)] +mod proc_macros_support; + /// Re-exports for proc-macro library to not require any additional /// dependencies to be explicitly added on the client side. #[doc(hidden)] @@ -80,6 +83,8 @@ pub mod __reexports { cfg_client_or_server! { pub use tokio; } + + pub use super::proc_macros_support::*; } pub use beef::Cow; diff --git a/core/src/proc_macros_support.rs b/core/src/proc_macros_support.rs new file mode 100644 index 0000000000..0eaa4b8edd --- /dev/null +++ b/core/src/proc_macros_support.rs @@ -0,0 +1,10 @@ +#[cold] +pub fn log_fail_parse(arg_pat: &str, ty: &str, e: &dyn std::fmt::Debug, optional: bool) { + let optional = if optional { "optional " } else { "" }; + tracing::debug!("Error parsing {optional}\"{arg_pat}\" as \"{ty}\": {e:?}"); +} + +#[cold] +pub fn log_fail_parse_as_object(e: &dyn std::fmt::Display) { + tracing::debug!("Failed to parse JSON-RPC params as object: {e}"); +} diff --git a/proc-macros/src/render_server.rs b/proc-macros/src/render_server.rs index e23418df3d..70c40a2392 100644 --- a/proc-macros/src/render_server.rs +++ b/proc-macros/src/render_server.rs @@ -350,59 +350,36 @@ impl RpcDescription { let params_fields_seq = params.iter().map(RpcFnArg::arg_pat); let params_fields = quote! { #(#params_fields_seq),* }; - let tracing = self.jrps_server_item(quote! { tracing }); + + let reexports = self.jrps_server_item(quote! { core::__reexports }); + let sub_err = self.jrps_server_item(quote! { SubscriptionCloseResponse }); let response_payload = self.jrps_server_item(quote! { ResponsePayload }); - let tokio = self.jrps_server_item(quote! { core::__reexports::tokio }); + let tokio = quote! { #reexports::tokio }; + let error_ret = if let Some(pending) = &sub { + quote! { + #tokio::spawn(#pending.reject(e)); + return #sub_err::None; + } + } else { + quote! { + return #response_payload::error(e); + } + }; // Code to decode sequence of parameters from a JSON array. let decode_array = { - let decode_fields = params.iter().map(|RpcFnArg { arg_pat, ty, .. }| match (is_option(ty), sub.as_ref()) { - (true, Some(pending)) => { - quote! { - let #arg_pat: #ty = match seq.optional_next() { - Ok(v) => v, - Err(e) => { - #tracing::debug!(concat!("Error parsing optional \"", stringify!(#arg_pat), "\" as \"", stringify!(#ty), "\": {:?}"), e); - #tokio::spawn(#pending.reject(e)); - return #sub_err::None; - } - }; - } - } - (true, None) => { - quote! { - let #arg_pat: #ty = match seq.optional_next() { - Ok(v) => v, - Err(e) => { - #tracing::debug!(concat!("Error parsing optional \"", stringify!(#arg_pat), "\" as \"", stringify!(#ty), "\": {:?}"), e); - return #response_payload::error(e); - } - }; - } - } - (false, Some(pending)) => { - quote! { - let #arg_pat: #ty = match seq.next() { - Ok(v) => v, - Err(e) => { - #tracing::debug!(concat!("Error parsing optional \"", stringify!(#arg_pat), "\" as \"", stringify!(#ty), "\": {:?}"), e); - #tokio::spawn(#pending.reject(e)); - return #sub_err::None; - } - }; - } - } - (false, None) => { - quote! { - let #arg_pat: #ty = match seq.next() { - Ok(v) => v, - Err(e) => { - #tracing::debug!(concat!("Error parsing \"", stringify!(#arg_pat), "\" as \"", stringify!(#ty), "\": {:?}"), e); - return #response_payload::error(e); - } - }; - } + let decode_fields = params.iter().map(|RpcFnArg { arg_pat, ty, .. }| { + let is_option = is_option(ty); + let next_method = if is_option { quote!(optional_next) } else { quote!(next) }; + quote! { + let #arg_pat: #ty = match seq.#next_method() { + Ok(v) => v, + Err(e) => { + #reexports::log_fail_parse(stringify!(#arg_pat), stringify!(#ty), &e, #is_option); + #error_ret + } + }; } }); @@ -447,43 +424,22 @@ impl RpcDescription { let destruct = params.iter().map(RpcFnArg::arg_pat).map(|a| quote!(parsed.#a)); let types = params.iter().map(RpcFnArg::ty); - if let Some(pending) = sub { - quote! { - #[derive(#serde::Deserialize)] - #[serde(crate = #serde_crate)] - struct ParamsObject<#(#generics,)*> { - #(#fields)* - } - - let parsed: ParamsObject<#(#types,)*> = match params.parse() { - Ok(p) => p, - Err(e) => { - #tracing::debug!("Failed to parse JSON-RPC params as object: {}", e); - #tokio::spawn(#pending.reject(e)); - return #sub_err::None; - } - }; - - (#(#destruct),*) + quote! { + #[derive(#serde::Deserialize)] + #[serde(crate = #serde_crate)] + struct ParamsObject<#(#generics,)*> { + #(#fields)* } - } else { - quote! { - #[derive(#serde::Deserialize)] - #[serde(crate = #serde_crate)] - struct ParamsObject<#(#generics,)*> { - #(#fields)* - } - let parsed: ParamsObject<#(#types,)*> = match params.parse() { - Ok(p) => p, - Err(e) => { - #tracing::debug!("Failed to parse JSON-RPC params as object: {}", e); - return #response_payload::error(e); - } - }; + let parsed: ParamsObject<#(#types,)*> = match params.parse() { + Ok(p) => p, + Err(e) => { + #reexports::log_fail_parse_as_object(&e); + #error_ret + } + }; - (#(#destruct),*) - } + (#(#destruct),*) } }; From 2f216b831a76627d3675d3dad1f627d2368ef79b Mon Sep 17 00:00:00 2001 From: DaniPopes <57450786+DaniPopes@users.noreply.github.com> Date: Tue, 11 Jun 2024 09:09:59 +0200 Subject: [PATCH 2/3] chore: extract panic functions --- core/src/proc_macros_support.rs | 21 +++++++-- proc-macros/src/render_client.rs | 10 ++-- proc-macros/src/render_server.rs | 78 +++++++++++++++++--------------- 3 files changed, 64 insertions(+), 45 deletions(-) diff --git a/core/src/proc_macros_support.rs b/core/src/proc_macros_support.rs index 0eaa4b8edd..9a30e8f48e 100644 --- a/core/src/proc_macros_support.rs +++ b/core/src/proc_macros_support.rs @@ -1,10 +1,23 @@ +use jsonrpsee_types::ErrorObjectOwned; + #[cold] -pub fn log_fail_parse(arg_pat: &str, ty: &str, e: &dyn std::fmt::Debug, optional: bool) { +pub fn log_fail_parse(arg_pat: &str, ty: &str, err: &ErrorObjectOwned, optional: bool) { let optional = if optional { "optional " } else { "" }; - tracing::debug!("Error parsing {optional}\"{arg_pat}\" as \"{ty}\": {e:?}"); + tracing::debug!("Error parsing {optional}\"{arg_pat}\" as \"{ty}\": {err}"); +} + +#[cold] +pub fn log_fail_parse_as_object(err: &ErrorObjectOwned) { + tracing::debug!("Failed to parse JSON-RPC params as object: {err}"); +} + +#[cold] +pub fn panic_fail_serialize(param: &str, err: serde_json::Error) -> ! { + panic!("Parameter `{param}` cannot be serialized: {err}"); } +#[cfg(debug_assertions)] #[cold] -pub fn log_fail_parse_as_object(e: &dyn std::fmt::Display) { - tracing::debug!("Failed to parse JSON-RPC params as object: {e}"); +pub fn panic_fail_register() -> ! { + panic!("RPC macro method names should never conflict. This is a bug, please report it."); } diff --git a/proc-macros/src/render_client.rs b/proc-macros/src/render_client.rs index e46c555e7c..800c70b81b 100644 --- a/proc-macros/src/render_client.rs +++ b/proc-macros/src/render_client.rs @@ -214,6 +214,8 @@ impl RpcDescription { let jsonrpsee = self.jsonrpsee_client_path.as_ref().unwrap(); let p = Ident::new(ILLEGAL_PARAM_NAME, proc_macro2::Span::call_site()); + let reexports = self.jrps_client_item(quote! { core::__reexports }); + if params.is_empty() { return quote!({ #jsonrpsee::core::params::ArrayParams::new() @@ -249,8 +251,8 @@ impl RpcDescription { quote!({ let mut #p = #jsonrpsee::core::params::ObjectParams::new(); #( - if let Err(err) = #p.insert( #params_insert ) { - panic!("Parameter `{}` cannot be serialized: {:?}", stringify!( #params_insert ), err); + if let Err(err) = #p.insert(#params_insert) { + #reexports::panic_fail_serialize(stringify!(#params_insert), err); } )* #p @@ -263,8 +265,8 @@ impl RpcDescription { quote!({ let mut #p = #jsonrpsee::core::params::ArrayParams::new(); #( - if let Err(err) = #p.insert( #params ) { - panic!("Parameter `{}` cannot be serialized: {:?}", stringify!( #params ), err); + if let Err(err) = #p.insert(#params) { + #reexports::panic_fail_serialize(stringify!(#params), err); } )* #p diff --git a/proc-macros/src/render_server.rs b/proc-macros/src/render_server.rs index 70c40a2392..f8e8057abf 100644 --- a/proc-macros/src/render_server.rs +++ b/proc-macros/src/render_server.rs @@ -107,6 +107,22 @@ impl RpcDescription { }) } + /// Helper that will ignore results of `register_*` method calls, and panic if there have been + /// any errors in debug builds. + /// + /// The debug assert is a safeguard should the contract that guarantees the method names to + /// never conflict in the macro be broken in the future. + fn handle_register_result(&self, tokens: TokenStream2) -> TokenStream2 { + let reexports = self.jrps_server_item(quote! { core::__reexports }); + quote! {{ + let _res = #tokens; + #[cfg(debug_assertions)] + if _res.is_err() { + #reexports::panic_fail_register(); + } + }} + } + fn render_into_rpc(&self) -> Result { let rpc_module = self.jrps_server_item(quote! { RpcModule }); @@ -121,18 +137,6 @@ impl RpcDescription { } }; - /// Helper that will ignore results of `register_*` method calls, and panic - /// if there have been any errors in debug builds. - /// - /// The debug assert is a safeguard should the contract that guarantees the method - /// names to never conflict in the macro be broken in the future. - fn handle_register_result(tokens: TokenStream2) -> TokenStream2 { - quote! {{ - let res = #tokens; - debug_assert!(res.is_ok(), "RPC macro method names should never conflict, this is a bug, please report it."); - }} - } - let methods = self .methods .iter() @@ -153,14 +157,14 @@ impl RpcDescription { if method.signature.sig.asyncness.is_some() { if method.with_extensions { - handle_register_result(quote! { + self.handle_register_result(quote! { rpc.register_async_method(#rpc_method_name, |params, context, ext| async move { #parsing #into_response::into_response(context.as_ref().#rust_method_name(&ext, #params_seq).await) }) }) } else { - handle_register_result(quote! { + self.handle_register_result(quote! { rpc.register_async_method(#rpc_method_name, |params, context, _| async move { #parsing #into_response::into_response(context.as_ref().#rust_method_name(#params_seq).await) @@ -172,14 +176,14 @@ impl RpcDescription { if method.blocking { quote!(register_blocking_method) } else { quote!(register_method) }; if method.with_extensions { - handle_register_result(quote! { + self.handle_register_result(quote! { rpc.#register_kind(#rpc_method_name, |params, context, ext| { #parsing #into_response::into_response(context.#rust_method_name(&ext, #params_seq)) }) }) } else { - handle_register_result(quote! { + self.handle_register_result(quote! { rpc.#register_kind(#rpc_method_name, |params, context, _| { #parsing #into_response::into_response(context.#rust_method_name(#params_seq)) @@ -223,14 +227,14 @@ impl RpcDescription { if sub.signature.sig.asyncness.is_some() { if sub.with_extensions { - handle_register_result(quote! { + self.handle_register_result(quote! { rpc.register_subscription(#rpc_sub_name, #rpc_notif_name, #rpc_unsub_name, |params, mut pending, context, ext| async move { #parsing #into_sub_response::into_response(context.as_ref().#rust_method_name(pending, &ext, #params_seq).await) }) }) } else { - handle_register_result(quote! { + self.handle_register_result(quote! { rpc.register_subscription(#rpc_sub_name, #rpc_notif_name, #rpc_unsub_name, |params, mut pending, context, _| async move { #parsing #into_sub_response::into_response(context.as_ref().#rust_method_name(pending, #params_seq).await) @@ -238,22 +242,22 @@ impl RpcDescription { }) } } else if sub.with_extensions { - handle_register_result(quote! { - rpc.register_subscription_raw(#rpc_sub_name, #rpc_notif_name, #rpc_unsub_name, |params, mut pending, context, ext| { - #parsing - let _ = context.as_ref().#rust_method_name(pending, &ext, #params_seq); - #sub_err::None - }) + self.handle_register_result(quote! { + rpc.register_subscription_raw(#rpc_sub_name, #rpc_notif_name, #rpc_unsub_name, |params, mut pending, context, ext| { + #parsing + let _ = context.as_ref().#rust_method_name(pending, &ext, #params_seq); + #sub_err::None }) + }) } else { - handle_register_result(quote! { - rpc.register_subscription_raw(#rpc_sub_name, #rpc_notif_name, #rpc_unsub_name, |params, mut pending, context, _| { - #parsing - let _ = context.as_ref().#rust_method_name(pending, #params_seq); - #sub_err::None - }) + self.handle_register_result(quote! { + rpc.register_subscription_raw(#rpc_sub_name, #rpc_notif_name, #rpc_unsub_name, |params, mut pending, context, _| { + #parsing + let _ = context.as_ref().#rust_method_name(pending, #params_seq); + #sub_err::None }) - } + }) + } }) .collect::>(); @@ -270,7 +274,7 @@ impl RpcDescription { .iter() .map(|alias| { check_name(alias, rust_method_name.span()); - handle_register_result(quote! { + self.handle_register_result(quote! { rpc.register_alias(#alias, #rpc_name) }) }) @@ -293,7 +297,7 @@ impl RpcDescription { .iter() .map(|alias| { check_name(alias, rust_method_name.span()); - handle_register_result(quote! { + self.handle_register_result(quote! { rpc.register_alias(#alias, #sub_name) }) }) @@ -303,7 +307,7 @@ impl RpcDescription { .iter() .map(|alias| { check_name(alias, rust_method_name.span()); - handle_register_result(quote! { + self.handle_register_result(quote! { rpc.register_alias(#alias, #unsub_name) }) }) @@ -353,15 +357,15 @@ impl RpcDescription { let reexports = self.jrps_server_item(quote! { core::__reexports }); - let sub_err = self.jrps_server_item(quote! { SubscriptionCloseResponse }); - let response_payload = self.jrps_server_item(quote! { ResponsePayload }); - let tokio = quote! { #reexports::tokio }; let error_ret = if let Some(pending) = &sub { + let tokio = quote! { #reexports::tokio }; + let sub_err = self.jrps_server_item(quote! { SubscriptionCloseResponse }); quote! { #tokio::spawn(#pending.reject(e)); return #sub_err::None; } } else { + let response_payload = self.jrps_server_item(quote! { ResponsePayload }); quote! { return #response_payload::error(e); } From eef7a7d2b445cd30f17cfa7ad33257f6cd13fc84 Mon Sep 17 00:00:00 2001 From: DaniPopes <57450786+DaniPopes@users.noreply.github.com> Date: Fri, 5 Jul 2024 12:26:04 +0200 Subject: [PATCH 3/3] docs: add a comment explaining the use of `#[cold]` --- core/src/proc_macros_support.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/core/src/proc_macros_support.rs b/core/src/proc_macros_support.rs index 9a30e8f48e..acb7f8264e 100644 --- a/core/src/proc_macros_support.rs +++ b/core/src/proc_macros_support.rs @@ -1,5 +1,8 @@ use jsonrpsee_types::ErrorObjectOwned; +// We're marking functions on the error paths as #[cold] to both reduce chance of inlining and to +// make the generated assembly slightly better. + #[cold] pub fn log_fail_parse(arg_pat: &str, ty: &str, err: &ErrorObjectOwned, optional: bool) { let optional = if optional { "optional " } else { "" };