Callback invoker to sink all thrown exceptions

[accelerated]

Provide a way to consistently invoke all callbacks which run on the librdkafka stack and prevent them from throwing. An error will be logged accordingly if a log callback is passed. Log callback is also guarded for throws.

[mfontanini]

Wouldn't it be better to use a simple function instead of this? I don't think there should be a need to indicate all template parameters when instantiating this, especially since this is always used internally. Something like this would work:

template <typename RetType, typename Callback, typename... Args>
RetType safe_invoke_callback(const std::string& callback_name,
                                                  const Callback& callback,
                                                  KafkaHandleBase* handle,
                                                  Args&&... args);

Which you can just invoke by doing:

safe_invoke_callback<int32_t>(callback_name, callback, nullptr,
                                                   topic, key, partition_count);

[accelerated]

I exposed the args because initially i was going to replace all the callback members with this wrapper so I wanted it to behave as much as possible as std::function does. Then I decided to only invoke it locally to minimize changes but the class structure remained. Your suggestion is simpler, I agree...however:

1. I personally prefer to keep settings and call parameters separate, not pass both in the function signature. It makes for a cleaner separation of intent and the design is more object oriented....even if it's an internal function.
2. Imagine later down the road you need to add an overload to safe_invoke_callback by adding an extra parameter. In the OO pattern, you can overload the constructor and be done with it. In the simple function case, adding an extra parameter probably will give you compile issues. Which overload will the compiler pick? Most likely it will pick overload#1 and consider your last parameter as Arg<0> in the param expansion pack. Would it work if you declare the overloads in reverse order? Who knows? Probably compiler-dependent, but I've had this issue in the past and the safest solution would be to have another function with another name, otherwise you may have the above problems, but then do you really want to have safe_invoke_callback2?

Perhaps a good compromise would be:

template <typename Func>
class CallbackInvoker {
    using RetType = typename Func::return_type;
    //constructor...
    template <typename ...Args>
    RetType operator()(Args&&... args); //auto deduct Args
}

Then you call it almost like the previous version but w/o arg specification
CallbackInvoker<AssignmentCallback>(...)(...);

Also I have a question for you. If the callback throws, the callback functor (or free function) will return false_value. That's fine as this is an error. However if the callback is empty, what do you return? False or true? In my code i decided to return true_value but perhaps this is something that could be passed as a constructor param?

if (callback) {
    return callback();
}
else {
   return ??? 
}

Returning true is fine, except there's cases like the SocketCallback where you have the following code

if (callback) {
    return callback();
}
else {
   return -1; //error or false
}

and in cases like the buffered_producer in the on_delivery_report you expect the callback to return true if it's not present:

bool should_produce = message.get_error() &&
                          (!produce_failure_callback_ || produce_failure_callback_(message));

[accelerated]

@mfontanini btw, on a totally unrelated tangent, I am never able to link the test application w/o using

target_link_libraries(cppkafka_tests cppkafka-test pthread rt ssl crypto dl z)
                                                           ^^^^^^^^^^^^^^^^^^^

I don't want to check this in, in case it may break other people's build but I wonder if I'm the only one with this issue. These are dependencies from librdkafka which I'm sure could be removed with compile time options, but what if other people need to use them? In that case your code does not link. I think having these will allow more flexibility for different compile settings that people might have.

[mfontanini]

Your arguments are usually very strawman-ish :). If you want to add a new parameter... you add it and then your compiler will complain because you're trying to call the callback with N+1 instead of N parameters. You go there and you fix it. This is an internal function so you know what you're doing. You'd have to change all invocations of the constructor in your case as well.

Anyway, I'm fine with just taking the callback and forwarding the parameters as you posted on your comment. I just don't think it's worth explicitly using all parameters and that fixes it so it's okay.

Regarding tests, this is probably how librdkakfa gets built where it uses features if it can basically. We can fix that. It will break on Windows but as it is now it's trying to link with pthread which also doesn't work there so it's already broken. Just add that and at some point that'll have to be fixed using FIND_PACKAGE to find openssl, threads, etc.

[accelerated]

Your arguments are usually very strawman-ish :)

That also works in relationships btw :).

Ok, wrt the callback. I'll keep the middle compromise proposal since I have most of the code as-is.
I'll also add the linker libs and when I get a bit of breathing room in my project I'll look into the find package alternative. I was curious as to how come you haven't had linker errors on your end when you test your solution...?

[mfontanini]

I'm not entirely sure why that happens. This also works fine in travis so it's not just my setup. I'm not sure how rdkafka decides to depend on libraries. Anyway, those sound like good defaults which everyone should have (at least in linux/osx).

[mfontanini]

Sorry, one last thing: can you move this inside include/cppkafka/detail? That's probably a better location for it, given this should only be used internally as a helper.

[accelerated]

done!