Possible concurrency issues in HttpRequestTemplate

[tbak]

HttpRequestTemplate builds Ribbon HttpRequest, and this code is executed concurrently by multiple client threads. This means that any change to HttpRequestTemplate must be safely published, so all threads have a consistent view of this class (Ribbon does not do any explicit synchronization for that).

We have a few issues in the current implementation:

• some fields are non-final, and when assigned we do not have a guarantee that they will be properly visible to other threads. This can be work around like in Ribbon proxy, where after HttpRequestTemplate object construction, it is assigned to a final field, and any explicit modifications after that are not allowed.
• there is some object state manipulation during client request execution (see below).

In the code snippet below:

@Override
public HttpRequestBuilder<T> requestBuilder() {
    if (setter == null) {
        setter = HystrixObservableCommand.Setter.withGroupKey(HystrixCommandGroupKey.Factory.asKey(clientName))
                .andCommandKey(HystrixCommandKey.Factory.asKey(name()));
    ...
}

If setter field is null, it will be constructed on-fly by a client thread, with no proper synchronization.

I think we should solve both issues. The latter one is pretty easy to fix. The former one much harder and might require extraction from HttpRequestTemplate a builder class and making the former fully immutable.

[allenxwang]

The intention of the HttpRequestTemplate is that it should be tightly controlled by APIs who create them and should never leak them to caller of the API. So it is not thread safe and is documented as such. The following code demonstrates the expected usage pattern of HttpRequestTemplate:

public class MovieService {
    private final HttpRequestTemplate<ByteBuf> template;

    public MovieService() {
        template = Ribbon.createHttpResourceGroup("mygroup").newHttpRequestTemplate("recommendation");
        template.withHttpMehtod(...)
                      .withUriTemplate(...);
        // call other setters of template
    }

    public RibbonRequest<ByteBuf> getRecommendations(int userId) {
        return template.newRequestBuilder().withRequestProperty("userId", userId).build();
    }
}

I agree it is better to have a builder to enforce immutability and thread safety once the template is constructed and necessary setters are called.

[NiteshKant]

I think we should look at making the HttpRequestTemplate as well as HttpResourceGroup thread-safe as it is easy to accidentally use it in a multi-threaded scenario.

One way of implementing immutability can be that you provide an explicit finish() method in the HttpRequestTemplate after which any write will create a new HttpRequestTemplate instance and return. This will not have runtime side-effects (exceptions) based on which path you create a HttpRequestTemplate instance but has an object allocation overhead if there is a mutation post finish()

The following sample elaborates the above:

        Ribbon.createHttpResourceGroup("mygrp")
              .newRequestTemplate("mytemp")
              .withHeader("Blah", "blah") // Updates the current template
              .finish() // creates a new immutable instance (copy) 
              .withHeader("blah1", "blah"); // Every update creates a new immutable template instance

[allenxwang]

I would prefer the builder approach since the finish() method. From API point of view, finish() cannot be easily enforced and object allocation post finish() is not obvious.

[allenxwang]

To fix the problem, the design of ResourceGroup is changed to have a builder class for RequestTemplate:

public abstract class ResourceGroup<T extends RequestTemplate<?, ?>> {
    // ...
    public static abstract class TemplateBuilder<S, R, T extends RequestTemplate<S, R>> {
        public abstract TemplateBuilder withFallbackProvider(FallbackHandler<S> fallbackProvider);

        public abstract TemplateBuilder withResponseValidator(ResponseValidator<R> transformer);

        /**
         * Calling this method will enable both Hystrix request cache and supplied external cache providers
         * on the supplied cache key. Caller can explicitly disable Hystrix request cache by calling
         * {@link #withHystrixProperties(com.netflix.hystrix.HystrixObservableCommand.Setter)}
         *
         * @param cacheKeyTemplate
         * @return
         */
        public abstract TemplateBuilder withRequestCacheKey(String cacheKeyTemplate);

        public abstract TemplateBuilder withCacheProvider(String cacheKeyTemplate, CacheProvider<S> cacheProvider);

        public abstract TemplateBuilder withHystrixProperties(HystrixObservableCommand.Setter setter);

        public abstract T build();
    } 

    public abstract <S> TemplateBuilder<S, ?, ?> newTemplateBuilder(String name, Class<? extends S> classType);
}

HttpResourceGroup itself will also have a builder:

public class HttpResourceGroup extends ResourceGroup<HttpRequestTemplate<?>> {

    public static class Builder {

        public Builder withClientOptions(ClientOptions options) {
            // ...
        }

        public Builder withHeader(String name, String value) {
            // ...
        }

        public HttpResourceGroup build() {
            // ...
        }
    }

    @Override
    public <T> HttpRequestTemplate.Builder newTemplateBuilder(String name, Class<? extends T> classType) {
         // ...
    }
}

Ribbon is changed to create HttpResourceGroup.Builder as opposed to HttpResourceGroup itself:

    public static Builder createHttpResourceGroupBuilder(String name);