Stop writing broken Go libraries

Some time ago my friends and I came up with an idea of merging our IRC bots and rewriting them in Go. In order to avoid rewriting most of the already existing functionality we attempted to find existing libraries supporting the web APIs used in the bot. One of the libraries needed in our project was a Reddit API library. This article was inspired by the first three libraries that I found and which I am not going to name here to avoid shaming their authors.

Every single one of those libraries had some fundamental problems that made it unusable in any real world applications. Furthermore every single one of those libraries was written in a way which made fixing the issues impossible without altering the existing library APIs in a non backwards compatibile way. As the same problems unfortunately plague many other Go libraries I attempted to list most of the sins commited by their authors below.

Do not hardcode the HTTP client

Many of the libraries contain the dreaded hardcoded reference to http.DefaultClient. While this would not be a problem in itself unfortunately library authors don't understand how the http.DefaultClient is supposed to be used. As the name of the default client suggests it should only be utilized when no other http.Client has been supplied by the user. Instead many library authors happily hardcode the references to http.DefaultClient in their code instead of only using it as a fallback. This causes issues which can make the library impossible to use under some conditions.

Firstly many of us read the article which states that the http.DefaultClient doesn't specify a timeout. When there is no guarantee that your HTTP requests will ever be completed (or at least may await a response for a completely unpredictable amount of time) you can expect strange goroutine leaks and unpredictable behaviour in your program. In my opinion this automatically makes every single library hardcoding the http.DefaultClient unusable.

Secondly there are many networking situations which require some extra configuration. Sometimes a proxy has to be used, an URL has to be slightly rewritten or maybe even http.Transport has to be replaced with a custom implementation. All of this can be easily achieved when a programmer can use their own http.Client instance with your library.

The recommended way of handling the http.Client in your library is to use the provided client but fallback to a default if needed:

func CreateLibrary(client *http.Client) *Library {
    if client == nil {
        client = http.DefaultClient
    }
    ...
}

Or if you want to remove this parameter from the factory function define a helper method in your struct and let the users set its property whenever they want:

type Library struct {
    Client *http.Client
}

func (l *Library) getClient() *http.Client {
    if l.Client == nil {
        return http.DefaultClient
    }
    return l.Client
}

Additionally people often feel the need to replace the http.Client with their own instance if some global behaviour is required for every request. This is a wrong approach - if you need to set some additional headers in your requests or introduce some kind of a common behaviour in your client simply set them for every request or wrap the custom client instead of replacing it completely.

Do not introduce global behaviour

An additional antipattern is allowing the user to set global behaviour in a library. An example of this would be allowing the user to set a global http.Client used by all HTTP calls performed in your library:

var libraryClient *http.Client = http.DefaultClient

func SetHttpClient(client *http.Client) {
    libraryClient = client
}

In general no package-global variables should exist in a library. When writing the code try to think what would happen if the user wanted to use the library multiple times in their program. Global behaviour makes it impossible to do so with different parameters. Moreover introducing global behaviour in your code leads to issues with testing and causes the code to have unnecessary complexity. Using global variables is likely to lead to unneeded dependencies between different parts of your program. Avoiding global state is especially important when writing library code.

Return structs not interfaces

This is one of the common problems (in fact I am also guilty of doing this at some point). A lot of libraries contain a following function:

func New() LibraryInterface {
    ...
}

In this case the struct implementing the behaviour of the library is hidden by returning an interface. In reality this should be replaced with:

func New() *LibraryStruct {
    ...
}

The interface declaration should be removed from the library unless it is used somewhere else as a function parameter. To help understand why this is the case think about the contract that you are creating when writing your library. When returning an interface you are basically declaring an exact set of available methods. If someone were to write their own mock implementation of that interface (for example for the purpose of testing) you are going to break their code by adding more methods to it. That means that while adding methods to library structs is safe, adding methods to interfaces is not. This idea is summed up well in the article Accept Interfaces Return Struct in Go. This approach also solves the issue of configuration. To adjust the behaviour of the library you can simply modify some of the fields exposed in its structs. That would not be possible if the library only provided the user with an interface.

To see this behaviour in real life have a look at Go's http.Client.

Use configuration structs to avoid modifying your APIs

Another approach to configuration is accepting a configuration struct in your factory function instead of passing configuration arguments directly to it. Thanks to that you can freely add new parameters to the library without breaking the existing API. All you need to do is to add a new field to the Config struct and ensure that there is a sane default behaviour in place.

func New(config Config) *LibraryStruct {
    ...
}

There is one scenario in which adding struct fields can backfire - if a user omits struct field names during initialization. Doing this is im my opinion an antipattern so I believe that breaking someone's code in this case can be forgiven. Maintaining compatibility is why you should write person{name: "Alice", age: 30} instead of person{"Alice", 30} in your code.

This approach can be seen in the supplementary golang.org/x/crypto package. Overall however I think that allowing the user to set various parameters in the returned struct is a better approach to configuration and this particular approach should only be used when writing complex methods.

Conclusion

As a rule of thumb you should always allow the user to specify their own http.Client when writing a library that performs HTTP calls. Additionally try to consider the impact of your future modifications by writing the code in an extensible way. Avoid global behaviour - libraries can't store global state. If you ever have any doubts - check what is going on in the standard library.

I think it is a good idea to test your library by using it in your program and asking yourself those questions:

  • what happens if you try to use the library multiple times?
  • is it possible to unit test the code which uses the library?
  • is it possible to expand the library in a non-invasive manner without breaking the existing code?
  • is it possible to add additional configuration parameters to the library without breaking the existing code?
2018-12-29