Skip to content

Replace Result returns in API with callback based error handling #70

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

Merged
merged 7 commits into from
May 18, 2016

Conversation

9prady9
Copy link
Member

@9prady9 9prady9 commented Apr 27, 2016

Fixes #45
Fixes #38

With this change, all functions and operations return appropriate
values/objects instead of wrapping those values inside a Result<>
object. All errors reported by FFI calls are handled by a global
error handling function that is registered by a call to the function
register_error_handler that accepts either a closure or a function.

By default, the error handler is set to a internal function that
prints the error message to standard output and then panics.

With this change, all functions and operations return appropriate
values/objects instead of wrapping those values inside a Result<>
object. All errors reported by FFI calls are handled by a global
error handling function that is registered by a call to the function
`register_error_handler` that accepts either a closure or a function.

By default, the error handler is set to a internal function that
prints the error message to standard output and then panics.
@9prady9 9prady9 added this to the 3.3.0 milestone Apr 27, 2016
@9prady9 9prady9 changed the title Replace Result returns in API with callback based error handling [REVIEW] Replace Result returns in API with callback based error handling Apr 27, 2016
@9prady9
Copy link
Member Author

9prady9 commented Apr 27, 2016

@arrayfire/rust-devel

Feedback please.


pub fn handle_error_general(error_code: AfError) {
match error_code {
AfError::SUCCESS => {}, /* No-op */
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: use the function (maybe move it outside of the trait impl) to get the &str 's here to avoid having code dupes:

impl Error for AfError {
    fn description(&self) -> &str {

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, will look into it.

@jramapuram Was wondering if global static HANDLE_ERROR should be mutex protected apart from being inside unsafe block ? Any idea about that ?

Copy link
Member

Choose a reason for hiding this comment

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

First question: why does it need to be unsafe? Its just a standard rust panic function.

Don't see a need for mutexing. There is no shared data in these handlers. They are independent.

Copy link
Member Author

Choose a reason for hiding this comment

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

Enclosing the code that mutates mutable static global inside an unsafe block is mandatory. Check here.

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok, that makes sense. In this case it would only need a mutex if you think people would be modifying the handler (at runtime) from multiple threads. If that is the case then yes it should be mutexed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we would need a static mutex if we need one at all.

Copy link
Member

Choose a reason for hiding this comment

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

True, but it is conceivable that the user wants a different method of error handling at a different point in the program execution and we should be robust in upholding that

Copy link
Member Author

Choose a reason for hiding this comment

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

True, agree with that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Regarding Error trait, i didn't notice. I have already implemented in defines module. Just need to reuse it.

Copy link
Member

Choose a reason for hiding this comment

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

Yup, what I suggested ;p

@jramapuram
Copy link
Member

@9prady9 : Looks good besides the comment above!

@9prady9
Copy link
Member Author

9prady9 commented Apr 28, 2016

@jramapuram StaticMutex is unstable and can't be used unless in Non-stable channels. I am not sure how else we can do it as of now. Other than that, i think i finished other changes.

@9prady9 9prady9 changed the title [REVIEW] Replace Result returns in API with callback based error handling Replace Result returns in API with callback based error handling Apr 28, 2016
@jramapuram
Copy link
Member

How about using lazy static:

#[macro_use]
extern crate lazy_static;

use std::sync::Mutex;

lazy_static! {
    static ref ARRAY: Mutex<Vec<u8>> = Mutex::new(vec![]);
}

fn do_a_call() {
    ARRAY.lock().unwrap().push(1);
}

fn main() {
    do_a_call();
    do_a_call();
    do_a_call();

    println!("called {}", ARRAY.lock().unwrap().len());
}

(http://stackoverflow.com/questions/27791532/how-do-i-create-a-global-mutable-singleton)

Add tests directory with a function to check the function
`register_error_handler`
@9prady9
Copy link
Member Author

9prady9 commented May 3, 2016

@jramapuram Check out the latest commit and let me know what you think.

use defines::AfError;
use std::error::Error;
use std::sync::Mutex;

pub type ErrorCallback = Fn(AfError);

pub static mut HANDLE_ERROR: &'static ErrorCallback = &handle_error_general;
Copy link
Member

Choose a reason for hiding this comment

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

@9prady9 : I don't think this does what you are expecting.
I would assume this would look like [havent tested] :

lazy_static! {
    static ref ARRAY: Mutex<ErrorCallback> = Mutex::new(&handle_error_general);
}

Then every call would be HANDLE_ERROR.lock().unwrap()(af_err)

This would ensure that when you swap out the HANDLE_ERROR function from a thread it would then lock the currently stored function (and thus ensure that the currently registered one would be called).

Copy link
Member Author

@9prady9 9prady9 May 3, 2016

Choose a reason for hiding this comment

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

Yes, i see it now, this is wrong. Won't help protect the data.

I have tried passing ErrorCallback to Mutex, but that didn't compile for me. Therefore, i created the mutex around an integer.

   Compiling arrayfire v3.3.0 (file:///E:/gitroot/arrayfire-rust)
<lazy_static macros>:30:7: 30:23 error: casting `usize` as `*const std::sync::mutex::Mutex<core::ops::Fn(defines::AfError) + 'static>` is invalid
<lazy_static macros>:30 $ T = 0 as * const $ T ; static mut ONCE : Once = ONCE_INIT ; ONCE . call_once
                              ^~~~~~~~~~~~~~~~
<lazy_static macros>:4:1: 5:73 note: in this expansion of lazy_static! (defined in <lazy_static macros>)
src\error.rs:10:1: 12:2 note: in this expansion of lazy_static! (defined in <lazy_static macros>)
error: aborting due to previous error
Could not compile `arrayfire`.

It seems like, internally lazy_static is casting the pointer to object to some integral type or something like that.

Copy link
Member

Choose a reason for hiding this comment

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

Can you try a reference to a function (instead of a function) or even a Box maybe?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think i tried reference, but that doesn't work because it will require life time specifier. Adding 'static life time brings us back to the old problem of not being able to create a Mutex around 'static life time object.

I don't remember trying Box but i suspect it will have similar problem. But will give it a shot.

Copy link
Member Author

@9prady9 9prady9 May 4, 2016

Choose a reason for hiding this comment

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

@jramapuram
Hm, i think i found the root cause of this. Fn(AfError) needs to have Send and Sync traits implemented for it to be used either in the context of Box or static mut.

I am getting the error explained in the output of rustc --explain E0117 if try to impl implement Send/Sync.

9prady9 added 2 commits May 4, 2016 16:10
Removed an incorrect implementation to provide thread safety
for the function register_error_handler.
Also, modifies binary operators to use Array objects instead of borrowed
references.
idxrs.set_index(&Seq::<f32>::default(), 1, Some(false));
let tmp = assign_gen(self as &Array, &idxrs, & $func(self as &Array, &rhs));
mem::replace(self, tmp);
Copy link
Member Author

Choose a reason for hiding this comment

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

@jramapuram Any idea if i am creating any leaks here ?

Copy link
Member

Choose a reason for hiding this comment

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

@9prady9 : sorry for the delay. According to the docs mem::replace does not deinitialize the old value. It does however return the previous value. I think de-initializing manually here is the apt choice to prevent a dangling pointer. I'm not too worried about the actual array since that is a smart pointer in C++. Thoughts @pavanky ?

@jramapuram
Copy link
Member

@9prady9 : So you can't implement Sync + Send for the lambda?

@9prady9
Copy link
Member Author

9prady9 commented May 11, 2016

@jramapuram Nope, doesn't seem like it. I tried doing something similar to the following

struct MyBox(*mut u8);

unsafe impl Send for MyBox {}
unsafe impl Sync for MyBox {}

for ErrorCallback type but the rust compiler keeps throwing out the following message

src\error.rs:24:1: 24:38 error: the impl does not reference any types defined in this crate; only traits defined in the current crate can be implemented for arbitrary types [E0117]
src\error.rs:24 unsafe impl Send for ErrorCallback {}
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src\error.rs:24:1: 24:38 help: run `rustc --explain E0117` to see a detailed explanation

@9prady9
Copy link
Member Author

9prady9 commented May 11, 2016

Putting the function pointer inside a struct compiled the code, but have to test this if it is doing what we need to do.

struct MyCB(*mut ErrorCallback);

unsafe impl Send for MyCB {}

I am in new areas (to me) of Rust now :)

UPDATE: ha, seems like if the variable type has fixed size during compile time, then we can do it. Hence the pointer stuff works.

@9prady9
Copy link
Member Author

9prady9 commented May 12, 2016

@jramapuram I came up with something like this and modified the test to look like this and trying to debug any issues with this.

@9prady9
Copy link
Member Author

9prady9 commented May 17, 2016

@arrayfire/rust-devel Please take a look at the latest changes for error handler thread safety and let me know if there are any issues.

Thanks.

// 1 => thread::sleep(Duration::from_millis(20)),
// 2 => thread::sleep(Duration::from_millis(10)),
// _ => (),
//}
Copy link
Member

Choose a reason for hiding this comment

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

unnecessary comments?

Copy link
Member Author

Choose a reason for hiding this comment

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

My bad, will remove them, i must have forgotten them after some debugging.

@jramapuram
Copy link
Member

Looks good @9prady9 !

@9prady9 9prady9 merged commit c6846d6 into devel May 18, 2016
@9prady9 9prady9 deleted the new_error_handling branch May 18, 2016 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants