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

Handle error on command for FFI client + C# example #122

Closed
Show file tree
Hide file tree
Changes from 7 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
38 changes: 31 additions & 7 deletions csharp/lib/AsyncClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -62,14 +62,12 @@ private void SuccessCallback(ulong index, IntPtr str)
});
}

private void FailureCallback(ulong index)
private void FailureCallback(ulong index, ErrorType error_type, IntPtr ptr)
Yury-Fridlyand marked this conversation as resolved.
Show resolved Hide resolved
{
var error = ptr == IntPtr.Zero ? null : Marshal.PtrToStringAnsi(ptr);
// Work needs to be offloaded from the calling thread, because otherwise we might starve the client's thread pool.
Task.Run(() =>
{
var message = messageContainer.GetMessage((int)index);
message.SetException(new Exception("Operation failed"));
});
_ = Task.Run(() => messageContainer.GetMessage((int)index)
.SetException(Errors.MakeException(error_type, error)));
}

~AsyncClient() => Dispose();
Expand All @@ -95,7 +93,13 @@ private void FailureCallback(ulong index)
#region FFI function declarations

private delegate void StringAction(ulong index, IntPtr str);
private delegate void FailureAction(ulong index);
/// <summary>
/// Glide request failure callback.
/// </summary>
/// <param name="index">Request ID</param>
/// <param name="error_type">Error type</param>
/// <param name="error">Error message</param>
private delegate void FailureAction(ulong index, ErrorType error_type, IntPtr error);
[DllImport("libglide_rs", CallingConvention = CallingConvention.Cdecl, EntryPoint = "get")]
private static extern void GetFfi(IntPtr client, ulong index, IntPtr key);

Expand All @@ -109,5 +113,25 @@ private void FailureCallback(ulong index)
[DllImport("libglide_rs", CallingConvention = CallingConvention.Cdecl, EntryPoint = "close_client")]
private static extern void CloseClientFfi(IntPtr client);

internal enum ErrorType : uint
{
/// <summary>
/// Represented by <see cref="Errors.UnspecifiedException"/> for user
/// </summary>
Unspecified = 0,
/// <summary>
/// Represented by <see cref="Errors.ExecutionAbortedException"/> for user
/// </summary>
ExecAbort = 1,
/// <summary>
/// Represented by <see cref="TimeoutException"/> for user
/// </summary>
Timeout = 2,
/// <summary>
/// Represented by <see cref="Errors.DisconnectedException"/> for user
/// </summary>
Disconnect = 3,
}

#endregion
}
33 changes: 33 additions & 0 deletions csharp/lib/Errors.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/**
* Copyright GLIDE-for-Redis Project Contributors - SPDX Identifier: Apache-2.0
*/

using static Glide.AsyncClient;

namespace Glide;

public abstract class Errors
{
public sealed class UnspecifiedException : Exception
{
internal UnspecifiedException(string? message) : base(message) { }
}

public sealed class ExecutionAbortedException : Exception
{
internal ExecutionAbortedException(string? message) : base(message) { }
}

public sealed class DisconnectedException : Exception
{
internal DisconnectedException(string? message) : base(message) { }
}

internal static Exception MakeException(ErrorType type, string? message) => type switch
{
ErrorType.ExecAbort => new ExecutionAbortedException(message),
ErrorType.Disconnect => new DisconnectedException(message),
ErrorType.Timeout => new TimeoutException(message),
_ => new UnspecifiedException(message),
};
}
27 changes: 20 additions & 7 deletions csharp/lib/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* Copyright GLIDE-for-Redis Project Contributors - SPDX Identifier: Apache-2.0
*/
use glide_core::connection_request;
use glide_core::errors::{error_message, error_type, RequestErrorType};
use glide_core::{client::Client as GlideClient, connection_request::NodeAddress};
use redis::{Cmd, FromRedisValue, RedisResult};
use std::{
Expand All @@ -22,7 +23,7 @@ pub enum Level {
pub struct Client {
client: GlideClient,
success_callback: unsafe extern "C" fn(usize, *const c_char) -> (),
failure_callback: unsafe extern "C" fn(usize) -> (), // TODO - add specific error codes
failure_callback: unsafe extern "C" fn(usize, RequestErrorType, *const c_char) -> (),
runtime: Runtime,
}

Expand Down Expand Up @@ -52,7 +53,7 @@ fn create_client_internal(
port: u32,
use_tls: bool,
success_callback: unsafe extern "C" fn(usize, *const c_char) -> (),
failure_callback: unsafe extern "C" fn(usize) -> (),
failure_callback: unsafe extern "C" fn(usize, RequestErrorType, *const c_char) -> (),
) -> RedisResult<Client> {
let host_cstring = unsafe { CStr::from_ptr(host as *mut c_char) };
let host_string = host_cstring.to_str()?.to_string();
Expand All @@ -78,7 +79,7 @@ pub extern "C" fn create_client(
port: u32,
use_tls: bool,
success_callback: unsafe extern "C" fn(usize, *const c_char) -> (),
failure_callback: unsafe extern "C" fn(usize) -> (),
failure_callback: unsafe extern "C" fn(usize, RequestErrorType, *const c_char) -> (),
) -> *const c_void {
match create_client_internal(host, port, use_tls, success_callback, failure_callback) {
Err(_) => std::ptr::null(), // TODO - log errors
Expand Down Expand Up @@ -118,7 +119,11 @@ pub extern "C" fn set(
let client = Box::leak(Box::from_raw(ptr_address as *mut Client));
match result {
Ok(_) => (client.success_callback)(callback_index, std::ptr::null()), // TODO - should return "OK" string.
Err(_) => (client.failure_callback)(callback_index), // TODO - report errors
Err(err) => {
logger_core::log_debug("command error", format!("callback {}, error {}, kind {:?}, code {:?}, category {:?}, detail {:?}", callback_index, err, err.kind(), err.code(), err.category(), err.detail()));

Choose a reason for hiding this comment

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

Doesn't err implement Debug or Display? I think we should be able to just print it without explicitly extracting the kind, code, category, and detail.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in 93186f3

let c_err_str = CString::new(error_message(&err)).expect("CString::new failed");

Choose a reason for hiding this comment

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

nit: I don't think we need expect here. We could probably use unwrap instead and it would be just as helpful wrt debug info. Applies throughout.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in 93186f3

(client.failure_callback)(callback_index, error_type(&err), c_err_str.as_ptr())
}
};
}
});
Expand All @@ -142,8 +147,12 @@ pub extern "C" fn get(client_ptr: *const c_void, callback_index: usize, key: *co
let client = unsafe { Box::leak(Box::from_raw(ptr_address as *mut Client)) };
let value = match result {
Ok(value) => value,
Err(_) => {
unsafe { (client.failure_callback)(callback_index) }; // TODO - report errors,
Err(err) => {
unsafe {
logger_core::log_debug("command error", format!("callback {}, error {}, kind {:?}, code {:?}, category {:?}, detail {:?}", callback_index, err, err.kind(), err.code(), err.category(), err.detail()));
let c_err_str = CString::new(error_message(&err)).expect("CString::new failed");
(client.failure_callback)(callback_index, error_type(&err), c_err_str.as_ptr())
};
return;
}
};
Expand All @@ -153,7 +162,11 @@ pub extern "C" fn get(client_ptr: *const c_void, callback_index: usize, key: *co
match result {
Ok(None) => (client.success_callback)(callback_index, std::ptr::null()),
Ok(Some(c_str)) => (client.success_callback)(callback_index, c_str.as_ptr()),
Err(_) => (client.failure_callback)(callback_index), // TODO - report errors
Err(err) => {
logger_core::log_debug("command error", format!("callback {}, error {}, kind {:?}, code {:?}, category {:?}, detail {:?}", callback_index, err, err.kind(), err.code(), err.category(), err.detail()));
let c_err_str = CString::new(error_message(&err)).expect("CString::new failed");
(client.failure_callback)(callback_index, error_type(&err), c_err_str.as_ptr())
}
};
}
});
Expand Down
Loading