Generating callbacks and a question on hooks

[Ignas Vyšniauskas]

Hi Anton,

Two rather long questions:

1. Wouldn't it be nicer if piqic generated behaviour's rather than header files with -spec's?

Since R15B you can specify typed callbacks. In my opinion it would be more elegant for piqic to generate a `foo_module_piqi_impl.erl` (vs. `*.HRL`) which would contain exactly the same things as the header file, except for `-spec` replaced by -callback`. It could even contain a default "stub" implementation as in the `foo_module_default_impl.erl`.

Pros:

* "morally" the right thing -- your callback module implements the RPC methods, so it implements the specified behaviour, consistent with OTP philosophy, etc.

* Dialyzer will be happy.

* At least some versions of `edoc` fail currently if you include the `.hrl` file, because it expects spec's next to the function definition.

* A small PITA will be fixed: the current HRL file imports the `foo_module_piqi.hrl` file, which means that if you have some other header which wants to depend on the types defined in the `user_piqi.hrl` you can't import both of these into a common module.

* in the future Erlang will probably support optional callbacks (such as `gen_server:format_status/2`), you could for now export them as "$(behaviour-name)_optional" or so, though currently there are no optional callbacks in piqi-rpc.

Cons:

* breaks support for pre-R15B. You could continue generating HRL files for that use case.

* can't really think of anything else

If you agree with this, I think I would be able to provide a patch for it.

2. You mentioned long ago that piqi 0.6.0 would add support for RPC method hooks. I don't seem to find any further information regarding this.

I suppose it did not happen? Do you have any further plans for this?

Actually, IMHO you shouldn't implement this, because that's not the responsibility of piqi-rpc. People can always add whatever custom hooks they want as part of the implementation module. But I am now working on a generic hook mechanism (on top of piqi RPC), so just wanted to check whether there's anything already out there.

The only problematic case is when you want to pass/retrieve transport-specific data (such as HTTP headers). Support for that could be improved, but I would argue it's best if this is simply passed/returned as an extra `transport_data` term from the implementation module, rather than as some kind of external hook, because in some cases this is some method-and-input specific data, rather than generic headers.

So e.g. you could have two functions of different arities for each method: one which only inputs/outputs the deserialised request data and one which also has as an extra argument of type `{transport_type(), [tranport_data()]}` and responds with `[transport_data()]`, so e.g.

    Input = {#foo_module_method_input{},  {http, [

        {http_headers, [

             {<<"Accept-Language">>, ...},

             {<<"Cookie">>, ...}

        }},

        ...

    }

And the response could be:

   Output = {{ok, #foo_module_method_output{}}, [{http_headers, [{<<"Age">>, 11}]}]}.

But it's hard to say if this is the cleanest / most generic solution.

Anyway, sorry for the long email and thanks for bringing us piqi!

--
Ignas

[Anton Lavrik]

Hi Ignas,

1. This is a great idea and your patch would be welcome. A couple of suggestions on how we could make it even better:

- we can put the behavior callbacks definitions directly in _piqi_rpc.erl

- we'll detect the otp version in piqic-erlang and generate either callbacks the new way or the -specs as we do it today in _piqi_impl.hrl

- I think it would be better to keep the _default_impl.erl as a separate file at least for now -- it will either include the _piqi_impl.hrl or use the typed behavior depending on the Erlang version piqic-erlang was running on

- later, we can get rid of _default_impl and put the default implementation stubs directly in _piqi_prc.erl but this will require renaming the callbacks to handle_<function_name> which is probably a worthwhile change anyway -- maybe for the next major release

- also, later, we can rename _piqi_impl.hrl to _piqi_rpc.hrl so that the whole thing look nicer (now thinking of it, can't really explain why I didn't do this from the beginning). we could even do this now as a part of this change, but keep the _piqi_impl.hrl as well for a version or two for backward compatibility.

What do you think?

As a side note, I believe the edoc requiring -specs next to the functions is an unfortunate change. Things like that make the language worse. In this case, it looks like they sacrificed flexibility for no good reason.

2. Honestly, I don't remember if I had plans for the piqi-rpc method hooks. There's nothing like it there for sure. I don't have a particular opinion about hooks, but if you think something can be generalized and made a part of piqi-rpc I'm open for discussion.

Anton

--
You received this message because you are subscribed to the Google Groups "piqi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to piqi+uns...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

[Ignas Vyšniauskas]

Hi Anton,

Thanks for the quick response.

On 10/10/2013 11:09 AM, Anton Lavrik wrote:
> 1. This is a great idea and your patch would be welcome. A couple of
> suggestions on how we could make it even better:
>

> <..>
>
> What do you think?

Agree with all your points. I hope to get round to writing the patch
pretty soon.

> As a side note, I believe the edoc requiring -specs next to the
> functions is an unfortunate change. Things like that make the
> language worse. In this case, it looks like they sacrificed
> flexibility for no good reason.

Indeed. The whole edoc suite is unfortunately quite broken.

> 2. Honestly, I don't remember if I had plans for the piqi-rpc method
> hooks. There's nothing like it there for sure. I don't have a
> particular opinion about hooks, but if you think something can be
> generalized and made a part of piqi-rpc I'm open for discussion.

I don't think it should be part of piqi-rpc. I just wanted to check the
status.

Just to prove I wasn't making this up, here's your email where you
mention it will be easy to add custom callbacks in piqi 0.6.0:

https://groups.google.com/forum/#!msg/piqi/aQYigMHdCkg/-HMvvykupAcJ

In particular you said:

>> 7) There is no callback structure that allows you to perform
>> actions AFTER (or BEFORE) callback invocation. For example Piqi
>> calls my login/1 function and after I return I have no control
>> anymore. This would be very useful to perform cross-cutting
>> operations such as logging/monitoring/authentication.
>
> With Piqi-0.6, it will become very easy to add custom features like
> that.

--
Ignas

[Anton Lavrik]

On Thu, Oct 10, 2013 at 3:12 AM, Ignas Vyšniauskas <bali...@gmail.com> wrote:

Hi Anton,

Thanks for the quick response.

On 10/10/2013 11:09 AM, Anton Lavrik wrote:
> 1. This is a great idea and your patch would be welcome. A couple of
>  suggestions on how we could make it even better:
>

> <..>
>
> What do you think?

Agree with all your points. I hope to get round to writing the patch
pretty soon.

Great!

 

> 2. Honestly, I don't remember if I had plans for the piqi-rpc method
>  hooks. There's nothing like it there for sure. I don't have a
> particular opinion about hooks, but if you think something can be
> generalized and made a part of piqi-rpc I'm open for discussion.

I don't think it should be part of piqi-rpc. I just wanted to check the
status.

Just to prove I wasn't making this up, here's your email where you
mention it will be easy to add custom callbacks in piqi 0.6.0:

https://groups.google.com/forum/#!msg/piqi/aQYigMHdCkg/-HMvvykupAcJ

In particular you said:

>> 7) There is no callback structure that allows you to perform
>> actions AFTER (or BEFORE) callback invocation. For example Piqi
>> calls my login/1 function and after I return I have no control
>> anymore. This would be very useful to perform cross-cutting
>> operations such as logging/monitoring/authentication.
>
> With Piqi-0.6, it will become very easy to add custom features like
> that.

Ah. Right. I meant that once the backend is rewritten in Erlang (already is -- I just need to release it as 0.6), it will become easier to add various customizations. One way to achieve something like that is to define another property in piqi.erlang.piqi, lets say, .erlang-before-function <callback-name>, and modify piqic_erlang_rpc to generate a call of this function before the main function handler.

This is just an example. It looks like you need more flexibility there so, yes, it doesn't have to be a part of piqi-rpc

Anton

[Motiejus Jakštys]

On Tue, Oct 8, 2013 at 2:25 PM, Ignas Vyšniauskas <bali...@gmail.com> wrote:
> So e.g. you could have two functions of different arities for each method:
> one which only inputs/outputs the deserialised request data and one which
> also has as an extra argument of type `{transport_type(),
> [tranport_data()]}` and responds with `[transport_data()]`, so e.g.
>
> Input = {#foo_module_method_input{}, {http, [
> {http_headers, [
> {<<"Accept-Language">>, ...},
> {<<"Cookie">>, ...}
> }},
> ...
> }
>
> And the response could be:
>
> Output = {{ok, #foo_module_method_output{}}, [{http_headers, [{<<"Age">>,
> 11}]}]}.
>
> But it's hard to say if this is the cleanest / most generic solution.

I dislike the API, but I am quite sure you already came up with
something different. Care to share it with us before implementation?

My nitpicks for the above:
1. pass Input (record) and ExtraStuff (proplist?) as two variables,
not one tuple.
2. Output API should support the following:
2.1. HTTP specific stuff (for instance, pass different return codes)
2.2. Non-HTTP specific error reporting. How do we pass errors if we
are making an TCP-PB interface?

Point 2.2 is becoming more relevant, because (as you know better than
me), we (Spil) are exposing some of the services via erlang term/pb
(piqi). I believe it will not take long until we switch some things on
into plain tcp/pb (piqi). I think API should have that in mind at this
stage.

--
Motiejus Jakštys

[Ignas Vyšniauskas]

Hi Motiejau,

On 10/14/2013 07:26 PM, Motiejus Jakštys wrote:
> On Tue, Oct 8, 2013 at 2:25 PM, Ignas Vyšniauskas

> <bali...@gmail.com> wrote: I dislike the API, but I am quite sure

> you already came up with something different. Care to share it with
> us before implementation?

To be honest, I'm not interested in this and don't plan to implement it.
It just wanted to note that it is something to be considered if generic
hooks/callbacks get re-considered at some point.

> I dislike the API, but I am quite sure you already came up with
> something different. Care to share it with us before implementation?
>

> My nitpicks for the above: <..>

I agree with your nitpicks. The approach is to just output transport
specific stuff and then the "transport handler" (HTTP/PB over TCP/etc)
just picks out what it understand out of it. Alternatively it can just
be wrapped to specify the transport [so e.g. it would become `{http,
[{http_headers, ...}]}` in the previous example] instead of a plain
proplist.

Re PB errors: errors are just data, right? There's nothing like HTTP
status codes and so on, you just encode them like everything else. Or am
I missing something? Piqi-rpc is already based on this.

--
Ignas

[Ignas Vyšniauskas]

On 10/10/2013 06:13 PM, Anton Lavrik wrote:
> On Thu, Oct 10, 2013 at 3:12 AM, Ignas Vyšniauskas
> <bali...@gmail.com <mailto:bali...@gmail.com>> wrote:
>
>> Agree with all your points. I hope to get round to writing the
>> patch pretty soon.
>
> Great!

Here's the PR: https://github.com/alavrik/piqi-rpc/pull/2

It's missing tests though, but I wanted to push this out to you as a
preview.

--
Ignas

[Anton Lavrik]

Thank you! I've added some comments there.

Anton