A simple RPC framework with protobuf service definitions

I think we could improve Twirp's routing scheme. It has two problems right now: the /twirp/ prefix, and the package.service scheme (instead of package/service).

Some people have mentioned to me that they're hesitant to use Twirp because the routes are prefixed with /twirp, which is concerning to them for several reasons:

• Trademarks: some very large organizations don't want to take any legal risks and are concerned that "twirp" could become trademarked.
• Feels like advertising: This was mentioned in #48: putting "twirp" in all your routes feels like it's just supposed to pump Twirp's brand.
• Homophonous with "twerp": In some Very Serious settings (like government websites), it's not okay that "twirp" sounds like "twerp", which means something like "insignificant pest."

We currently have the /twirp prefix to avoid colliding with gRPC's routes, which live at /package.service/method, and to reserve a space for future APIs (like a reflection server).

Slashes-based routes (instead of dot-based) would be preferable because lots of existing technology expects slash-based routing. Load balancers and proxies use slashes for namespaces. If Twirp used a slash after the package name, users could leverage this much more easily. This is often possible with dots, just annoying, as you have to write a (possibly pretty complex) regex.

I think we can fix both of these in one go. This would be a breaking change at the protocol level, so it's something to take very seriously, but I think we can do it without much pain. It would just take a version bump to v6.

Proposed Implementation:

I propose we use a new routing scheme: /<package>/<service>/<method>.

This does not collide with gRPC's routes, which is good. I think it gives us flexibility to put future routes in by putting them under a Twirp package. We could provide a reflection service like this one, which lists available services:

syntax = "proto3";

package twirp.meta

import "github.com/golang/protobuf/protoc-gen-go/descriptor/descriptor.proto"

service Reflector {
  rpc ListServices(ListServiceRequest) returns (ListServiceResponse);
}

message ListServiceRequest {}
message ListServiceResponse {
  repeated protobuf.ServiceDescriptorProto services = 1;
}

It would live at /twirp.meta/Reflector, which seems good to me.

Regarding compatibility: generated clients would always use the new routing scheme. Generated servers would be able to use both, in case your users have old generated clients still around. We would accomplish this by exporting two path prefixes:

const (
  HaberdasherPathPrefix = "/notreal.example/Haberdasher/"
  HaberdasherLegacyPathPrefix = "/twirp/notreal.example.Haberdasher/"
)

Users can mount both on a mux if they want to support old clients. If not, they can just use the new one.

If we do this, there are other consequences.

• Documentation would need to be updated everywhere.
• Server implementations in other languages would need to be updated ASAP, as new clients won't work with old servers.
• If people have log parsing or metrics gathering code that operates on routes, they'd need to change them.

The pain of those other consequences is a good reason that we should try to get this out promptly, if we do it.

The way URLs are defined makes one believe the exact name of service is accepted. https://github.com/twitchtv/twirp/blob/03ac4c/docs/spec_v5.md#urls

In practice however both service and method names are changed. https://github.com/twitchtv/twirp/blob/03ac4c/protoc-gen-twirp/generator.go#L956

Should I add this to the spec for clarity and such that others do not get blind-sided by it while implementing?

@khevse discovered it in twirpy I believe at least the rust, swagger, TypeScript, Dart and php implementations suffer from this.

Aside: The method itself is marked internal API. It might be easier for implementations if it were public https://github.com/twitchtv/twirp/blob/03ac4c/internal/gen/stringutils/stringutils.go#L71

#3 lays out some ideas for how Twirp could support streaming RPCs on the wire. Equally important is how we support them in generated code.

This issue is for designing the generated Go code. When we've landed on a proposal that seems pretty good, I'll edit this top text with what we've got, and then we can look at implementing it.

Let's use this service to discuss what things would look like:

syntax = "proto3";

service Streamer {
  rpc Transaction(Req) returns (Resp);
  rpc Upload(stream Req) returns (Resp);
  rpc Download(Req) returns (stream Resp);
  rpc Bidirectional(stream Req) returns (stream Resp);
}

message Req {}
message Resp {}

Similar to how the server-side hooks area thing, it'd be beneficial to have client-side hooks as well for automating things such as client-side logging, retries, etc.

Here's a rough draft of the API that I've thought up.

type ClientHooks struct {
  // RequestPrepared is called as soon as a request has been created and before it has been sent 
 // to the Twirp server.
  RequestPrepared func(context.Context, *http.Request) (context.Context, error)

 // RequestFinished (alternatively could be named ResponseReceived and take in some response from 
 // the server as well) is called after a request has finished sending. Since this is terminal, the context is 
 // not returned. 
 RequestFinished func(context.Context)

 // Error hook is called whenever an error occurs during the sending of a request. The Error is passed
 // as an argument to the hook.
  Error func(context.Context, twirp.Error) context.Context 
}

Looking forward to hearing y'alls thoughts!

Add support for a "TooManyRequests" Twirp error code, which will get translated to the equivalent 429 HTTP error code.

Why call this TooManyRequests instead of RateLimitExceeded?

I'm not super particular with the exact naming here and would be happy to move to something like RateLimitExceeded if others think this is better, but on initial glance exactly matching the corresponding code from the HTTP spec seemed valuable.

Why not use the existing ResourceExhausted error code to indicate the client should slow down

Technically, exceeding a rate limit could be considered "exhausting a per-user quota" and be represented by the existing ResourceExhausted error. However, on first glance, my interpretation of receiving a ResourceExhausted error code as a client caller is that something is problematic or damaged on the server, not that my request pattern as a client is faulty. In fact, the Twirp service may be completely healthy, but choose to reject a client's traffic because it breaks some negotiated rate limit, even if it could handle the traffic just fine without exhausting its resources. In addition, there are many potential situations where there are no user-level limits and yet rate limiting is still expected. For example, a Twirp service may place global rate limit across all callers on a very expensive API operation to ensure a downstream remains healthy.

Secondly, breaking this out into its own error code allows the very simple creation of rate-limiting Twirp middleware via ServerHooks. If we chose to reuse the existing ResourceExhausted error code, any middleware would have to inspect both the error code as well as actual error message to ensure that the ResourceExhausted error received was due to rate limiting instead of some other generic resource exhaustion. Checking the error message directly is more brittle to changes.

In my mind, having a error code that either strictly or loosely correlates with the 429 TooManyRequests error code from the HTTP spec is valuable for both Twirp clients, service developers and the Twirp ecosystem as a whole.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

I'll keep this pretty short for now (I also know we discussed this very briefly @spenczar) but just to get the conversation going since this is on my near-term radar:

• It would be nice to be able to have twitchtv/twirp generated code in a different package than the golang/protobuf generated code. This in theory would allow you to have separation of your Protobuf definitions and the specific RPC protocol you use. For example, I'd love to have:

• foo/barpb - Protobuf messages for package foo.bar

• foo/twitchbarpb - Twirp client/servers for package foo.bar

• foo/grpcbarpb - gRPC client/servers for package foo.bar - I'm not sure if this is will be allowed with the new protoc-gen-go-grpc via https://github.com/protocolbuffers/protobuf-go/blob/master/compiler/protogen/protogen.go but we can leave this out of it

• foo/magicbarpb - My custom magical RPC implementation (not in reality but as an example)

What this would require is protoc-gen-twirp recognizing when it was generating code in a different package than the package of the golang/protobuf generated code, and then generating imports, ie:

// all shorthand, not even importing context
import barpb "github.com/alice/bob/foo/barpb"

type EchoService interface {
  Echo(context.Context, *barpb.EchoRequest) (*barpb.EchoResponse, error)
}

This might be able to be implemented with some combination of the Mkey=value (which would be annoying), import_path, and import_prefix options, but honestly it would be so much easier if there was just a definition_path=github.com/alice/bob/foo/barpb option or something similar to just denote this easily. There are Golang Protobuf plugin libraries that handle this kind of behavior, so there's a lot of work to pattern off of.

Thoughts?

Hello,

I was really excited to read about twirp and want to undertake a project with it. Unfortunately, twirp seems to make some pretty opinionated decisions about my API's pathing. I've noticed that the generator prepends the literal "twirp" to the url, and seems to require the services internal name be exposed. I'm not familiar with writing proto generators but I assume it is possible to provide some sort of generator option to disable or customize this functionality.

For the hard coded /twirp, I'm wondering if this is a twitch specific thing or if you're open to removing it. Considering I can use my own mux, I'm capable of prepending paths already.

For the service name, it seems a bit less straight forward. It is possible to expose the handlers so they can be plugged directly into the mux. I guess the apprehension is keeping twirp from becoming a complex mux in and of itself, but I think it can be made possible without convoluting the code base.

Example: https://github.com/yarpc/yarpc-go/blob/dev/internal/protoplugin/runner.go#L50

Will be needed when request/response types are outside of the package of the service, or if generating the Twirp handlers to a different golang package.

Nice work by the way!

The generated twirp servers include import log "log"

They're used in error cases like this:

	if _, err = resp.Write(buf.Bytes()); err != nil {
		log.Printf("errored while writing response to client, but already sent response status code to 200: %s", err)
	}

It would be great if I could provide my own logger of some kind.

One proposed API would be to expose an optional function I could type assert for.

func (s *myServer) SetLogger(logger *log.Logger) {
  s.logger = logger
}

The client code could then call

x := NewMyServer(...)
x.(twirp.Logthingie).SetLogger(myOwnLogger)

If you're ambitious, you could have a specific twirp type logger for more strongly typed information

type TwirpLogger interface {
  ErrorWritingReponse(ctx context.Context, writeErr err)
}

My implementation could then extract the method name from the ctx.

This is a proposal for an improvement to how Twirp handlers return errors. I originally brought this up a couple of years ago on the Twirp Slack in a discussion with @spenczar and @marwan-at-work.

To summarize, I propose that we add the following interface to the twirp package. A detailed proposal with background information follows.

// ErrorCoder may be implemented by third-party error types returned by
// Twirp handler functions to indicate a Twirp error code to report.
//
// If a non-nil error e returned by a handler is not an Error but is an
// ErrorCoder (or wraps one as indicated by errors.As), the server responds
// with an error using the code given by e.TwirpErrorCode and a message
// given by e.Error.
type ErrorCoder interface {
	TwirpErrorCode() ErrorCode
}

If this proposal is acceptable, I'm happy to send a PR.

Background

At my company we use Twirp extensively for making RPCs across systems. We sometimes need to convey particular error conditions (entity not found, say) and handle them differently on the client side. We can do this by sending a particular Twirp error code as the response.

The way this works today is that if a handler returns an error that implements twirp.Error, that Twirp error is used for the response. If the handler returns a non-nil error that does not implement twirp.Error, then it is considered a generic internal error and wrapped using twirp.InternalErrorWith.

This works well enough if the handler makes decisions about different error conditions directly. However, if the error is created a few functions down the call chain, it works less well:

• The function that generates the error might not be Twirp-specific, so it's not appropriate to return a twirp.Error, which is very much Twirp-specific. For instance, we have some servers that have shared internal logic which is used for generating web pages as well as Twirp responses and we don't want to generate Twirp errors in non-Twirp routes.
• The error generated down the call stack might be wrapped with other errors before the handler route gets it.

To work around this issue, a common pattern is for our servers to use a function to match errors to various internal error variables/types and return the appropriate Twirp error:

func toTwirpError(err error) error {
	switch {
	case errors.Is(err, errJobFinished):
		return twirp.NewError(twirp.FailedPrecondition, err.Error())
	case errors.Is(err, errNoLogDataAfterWait):
		return twirp.NewError(twirp.DeadlineExceeded, err.Error())
	case errors.As(err, new(jobNotFoundError)):
		return twirp.NotFoundError(err.Error())
	// ... several more cases ...
	default:
		return err
	}
}

All handlers have to remember to call toTwirpError any time they are returning the results of some function call that might return one of these internal errors. This is tedious and easy to mess up. It also happens to be fairly inefficient.

Proposal

We propose to add the ErrorCoder interface described above.

The idea is that the generated handler code will look something like this:

func writeError(ctx context.Context, resp http.ResponseWriter, err error, hooks *twirp.ServerHooks) {
	// If we already have a twirp.Error, use it.
	twerr, ok := err.(twirp.Error)
	if !ok {
		var ec twirp.ErrorCoder
		if errors.As(err, &ec) {
			// If we have a twirp.ErrorCoder, use the error code it
			// provides.
			twerr = twirp.NewError(ec.TwirpErrorCode(), err.Error())
		} else {
			// Otherwise, non-twirp errors are wrapped as Internal by default.
			twerr = twirp.InternalErrorWith(err)
		}
	}
	// ...
}

This allows us to create internal error types for each project which which are not Twirp-specific but can be turned into Twirp errors because they correspond to a Twirp error code. We can freely return those from any function whether or not it's being called as part of a Twirp route and even wrap them as they go up the call stack.

Backward compatibility

This is fully backward compatible and doesn't change the behavior of any existing code.

(Technically speaking, it could change the behavior of code which uses an error type which has a method TwirpErrorCode() twirp.ErrorCode, but that seems very unlikely.)

Alternatives

One slight variant on this interface which would be slightly more flexible but require a little more boilerplate to implement would be to have the optional method return a Twirp Error rather than just the ErrorCode:

type Errorer interface {
	TwirpError() Error
}

Naming

I think it's important that the method is called TwirpErrorCode, not ErrorCode, because the purpose of the interface is to be implemented by types outside of a Twirp context and so it's good for the method to make it clear that it is a Twirp-related adapter.

If the method is TwirpErrorCode, you could then argue that the interface should be TwirpErrorCoder, though it's a little redundant with the package name: twirp.TwirpErrorCoder. I don't have a strong feeling about this, though. (The interface itself should rarely be named in any code.)

Relates to #84. My organization has some tooling that expects lowerCamelCase fields and Twirp currently does not allow you configure this behavior. Mirrors implementation of similar option added in https://github.com/twitchtv/twirp/pull/271.

Hi everyone,

I want to discuss a minor suggestion in the documentation - which Is why I first created this issue instead of directly creating a pull request with the documentation changes.

This section in the documentation explains how one can send simple requests against a twirp protobuf endpoint using a combination of curl and protoc. While it works, it seems slightly cumbersome as a command line experience.

To make life easier, I've created the open-source command line tool protoCURL. This CLI adds some convinience whlie enabling one to write JSON or Protobuf Text Format when interacting with a Protobuf HTTP REST endpoint.

The example from the docs

echo 'subject:"World"' \
	| protoc --encode example.helloworld.HelloReq ./rpc/helloworld/service.proto \
	| curl -s --request POST \
      --header "Content-Type: application/protobuf" \
      --data-binary @- \
      http://localhost:8080/twirp/example.helloworld.HelloWorld/Hello \
	| protoc --decode example.helloworld.HelloResp ./rpc/haberdasher/service.proto

would then be written instead as

protocurl -I rpc/helloworld -i ..HelloReq -o ..HelloResp \
  -u http://localhost:8080/twirp/example.helloworld.HelloWorld/Hello \
  -d 'subject:"World"' -q

The second form is smaller and easier to work with - which is also the reason why I created the CLI. It handles the protobuf conversions while allowing one to write directly in the protobuf text format / JSON. It goes through the messages in the proto files directory and searches for the (unique) message type definitions given the paths ..HelloReq and ..HelloResp (where .. stands for "please infer the full path for me"). You can find more examples of the CLI usage here.

What do you think? Perhaps it could make sense, to add-to/replace the way of interacting with the twrip endpoints to the docs?

Disclaimer: I have not used Twirp in any project - but while looking for solutions to this problem (before creating protocurl) - I had found your documentation. Since there is a solution for this exact problem, you might be interested in integrating it.

Thanks for your attention!

Issue #371:

Description of changes: generator.go in func doJSONRequest was changed from

	t.P(`  marshaler := &`, t.pkgs["protojson"], `.MarshalOptions{UseProtoNames: true}`)

to

	t.P(`  marshaler := &`, t.pkgs["protojson"], `.MarshalOptions{UseProtoNames: !s.jsonCamelCase, EmitUnpopulated: !s.jsonSkipDefaults}`)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Hello there! We're using the twirp library at work for sending/receiving JSON requests via http. It has come to our attention that the omitempty flag is not supported when it comes to sending JSON requests. For now, our solution is to patch it manually after the code is being generated. The bug seems to be in protoc-gen-twirp/generator.go at func doJSONRequest and should be adapted in line 824 from

	t.P(`  marshaler := &`, t.pkgs["protojson"], `.MarshalOptions{UseProtoNames: true}`)

to

	t.P(`  marshaler := &`, t.pkgs["protojson"], `.MarshalOptions{UseProtoNames: !s.jsonCamelCase, EmitUnpopulated: !s.jsonSkipDefaults}`)

Thanks in advance!

First off, thanks so much for twirp!

The OpenTelemetry project has a repo containing autoinstrumenters for widely used packages (like net/http and others). This issue is to track work on creating auto-instrumentation for twirp. I'm interested to hear your thoughts. Is this something you would be open to for this project?

Hello, I am fairly new to Twirp so this may be something I am doing wrong. We recently upgraded from github.com/twitchtv/twirp/[email protected] to github.com/twitchtv/twirp/[email protected].

We have a service called Builder which we are instantiating in the router.go of our application using:

builderHandler := rpc.NewBuilderServer(d.handlers.builder, tw.DefaultOptions(), twirp.WithServerPathPrefix(pathPrefix))

Here is the definition of tw.DefaultOptions():

func DefaultOptions() twirp.ServerOption {
	options := func(o *twirp.ServerOptions) {
		o.JSONSkipDefaults = true
		o.Hooks = defaultHooks()
	}
	return options
}

The NewBuilderServer Twirp function gets defined as follows in v8.1.2:

func NewBuilderServer(svc Builder, opts ...interface{}) TwirpServer {
	serverOpts := newServerOpts(opts)

	// Using ReadOpt allows backwards and forwads compatibility with new options in the future
	jsonSkipDefaults := false
	_ = serverOpts.ReadOpt("jsonSkipDefaults", &jsonSkipDefaults)
	jsonCamelCase := false
	_ = serverOpts.ReadOpt("jsonCamelCase", &jsonCamelCase)
	var pathPrefix string
	if ok := serverOpts.ReadOpt("pathPrefix", &pathPrefix); !ok {
		pathPrefix = "/twirp" // default prefix
	}

	return &builderServer{
		Builder:          svc,
		hooks:            serverOpts.Hooks,
		interceptor:      twirp.ChainInterceptors(serverOpts.Interceptors...),
		pathPrefix:       pathPrefix,
		jsonSkipDefaults: jsonSkipDefaults,
		jsonCamelCase:    jsonCamelCase,
	}
}

The issue here is that, as we can see in the definition of tw.DefaultOptions(), jsonSkipDefaults should be set to true. However, _ = serverOpts.ReadOpt("jsonSkipDefaults", &jsonSkipDefaults) does not change the value of jsonSkipDefaults and leaves it as false.

For context here is how the NewBuilderServer Twirp function gets defined in v8.0.0:

func NewBuilderServer(svc Builder, opts ...interface{}) TwirpServer {
	serverOpts := twirp.ServerOptions{}
	for _, opt := range opts {
		switch o := opt.(type) {
		case twirp.ServerOption:
			o(&serverOpts)
		case *twirp.ServerHooks: // backwards compatibility, allow to specify hooks as an argument
			twirp.WithServerHooks(o)(&serverOpts)
		case nil: // backwards compatibility, allow nil value for the argument
			continue
		default:
			panic(fmt.Sprintf("Invalid option type %T on NewBuilderServer", o))
		}
	}

	return &builderServer{
		Builder:          svc,
		pathPrefix:       serverOpts.PathPrefix(),
		interceptor:      twirp.ChainInterceptors(serverOpts.Interceptors...),
		hooks:            serverOpts.Hooks,
		jsonSkipDefaults: serverOpts.JSONSkipDefaults,
	}
}

This works as expected. These two pieces of code are semantically identical for our purposes. I was wondering if I could get some help in determining what could be causing an issue like this to occur. Any guidance will be greatly appreciated!

If this is intended behavior, my bad. I tried searching through issues and I couldn't find much.

I have the following proto file (where I have just services defined):

// file services.proto
syntax = "proto3";

package rpc;
option go_package = "github.com/lrstanley/spectrograph/internal/rpc";

import "google/protobuf/empty.proto";
import "internal/models/protos/servers.proto";
import "internal/models/protos/worker.proto";

service Worker {
    rpc Health(google.protobuf.Empty) returns (models.WorkerHealth); // models.WorkerHealth being from worker.proto
    rpc UpdateServer(models.ServerDiscordData) returns (google.protobuf.Empty); // models.ServerDiscordData being from servers.proto
    rpc UpdateServerStatus(models.ServerStatus) returns (google.protobuf.Empty); // models.ServerStatus being from servers.proto
}

It has two imports:

import "internal/models/protos/servers.proto";
import "internal/models/protos/worker.proto";

Both of these imports are within the same directory of eachother, but not in the directory of the services/twirp files, and both actually specify the same go_package and package line, as they're only split into multiple files given the size and specific-scope of the messages:

// file: servers.proto
syntax = "proto3";

package models;
option go_package = "github.com/lrstanley/spectrograph/internal/models";

[... server-specific messages defined below...]

// file: worker.proto
syntax = "proto3";

package models;
option go_package = "github.com/lrstanley/spectrograph/internal/models";

[... worker-specific messages defined below...]

I am able to import both and use them successfully. However, I noticed the generated *.twirp.go files have:

import models "github.com/lrstanley/spectrograph/internal/models"
import models1 "github.com/lrstanley/spectrograph/internal/models"

And reference the same go_package as multiple imports throughout the file, which can be rather confusing.

This works successfully (it's supported behavior by protobuf, and from my understanding, not discouraged), however I'm wondering if we can add some logic that ensures if there are multiple imports with the same exact go package name and path, that only one line is imported?

The more serious issue I suspect is: if the current behavior could cause issues for those that use init() functions in their Go packages, where the *.pb.go files are generated, as I suspect these would get executed twice. I.e. if variables or similar are initialized, this could cause weird state issues for users, where two sets of initialized state are used.