nrpc icon indicating copy to clipboard operation
nrpc copied to clipboard

Use pointers for Server/Client args and return values

Open euforic opened this issue 7 years ago • 8 comments

Returning a pointer is nice because if the function has an error most times you do not want to return an empty response struct

Below is an example of how gRPC func signatures look

type routeGuideServer struct {
        ...
}
...

func (s *routeGuideServer) GetFeature(ctx context.Context, point *pb.Point) (*pb.Feature, error) {
        ...
}
...

func (s *routeGuideServer) ListFeatures(rect *pb.Rectangle, stream pb.RouteGuide_ListFeaturesServer) error {
        ...
}
...

func (s *routeGuideServer) RecordRoute(stream pb.RouteGuide_RecordRouteServer) error {
        ...
}
...

func (s *routeGuideServer) RouteChat(stream pb.RouteGuide_RouteChatServer) error {
        ...
}

euforic avatar Jan 25 '18 00:01 euforic

Plus all of the get methods on the gRPC generated types are only defined on pointer receivers.

alittlebrighter avatar Apr 16 '18 05:04 alittlebrighter

Makes sense. @mdevan do you agree ? If so we would welcome a PR.

cdevienne avatar Sep 19 '18 13:09 cdevienne

What about the service/method subject params? They should probably remain plain strings.

Agree that other args/receivers should be pointers.

mdevan avatar Sep 19 '18 15:09 mdevan

Subject params should remain strings indeed.

cdevienne avatar Sep 19 '18 15:09 cdevienne

IMO it is not as simple as it sounds especially on the return side.

Pure NATS client calling convention results in client like this (from my code):

func (c *FinClient) BookUpdate(req *Book) (resp BookResponse, err error) {
	err = c.ec.Request(BookUpdateSubj, req, &resp, c.timeout)
	return
}

Note, that NATS takes pointer to a struct to fill for the response object. That means someone has to allocate it. If caller is to allocate the struct, then response can only be returned as input argument.

So, there is a choice to make:

  1. Use idiomatic calling convention and return a struct.
  2. Take return value as a pointer just like NATS does.

kulak avatar May 20 '20 13:05 kulak

The way nats EncodedConn handle return types is required because Request is generic and there is no other way for it to allocate the response itself.

In nrpc we generate the code, so we can generate code the allocate the response easily.

Passing the return value as a pointer has 2 cons for the developer:

  • Declare the response variable for each request
  • Always know the exact response type before calling the function

So the choice remains:

  1. Return plain structure
  2. Return pointer to structure

I think returning pointers has only advantages and will have little impact for the existing code.

Taking pointers for the arguments on the other hand, will have consequences for the existing code, so we could make this behavior optional (or have an option to keep the old behavior)

cdevienne avatar May 22 '20 08:05 cdevienne

All protobuf messages must be passed and returned by reference - protobuf implementation requires that by making the messages (indirectly) contain pragma.DoNotCopy. That does not enforce by-reference passing, but is caught by go vet

ademenev avatar Aug 10 '20 03:08 ademenev

This is a very good point

cdevienne avatar Aug 12 '20 08:08 cdevienne

It's been a while but it is now implemented

cdevienne avatar Mar 31 '23 15:03 cdevienne