Per request timeout options

[devonhumes]

We have an API that we are using that expects us to use long polling for a GET request. We can change the timeouts to support that, but there are other calls in the API that don't support long polling and we want the timeouts to be smaller for those calls. Is there a way to support this with Feign?

[codefromthecrypt]

There's no way to do this in the same interface. You'd have to create multiple instances of feign, one for each deviation to Request.Options.

[codefromthecrypt]

well. that or make a Client wrapper which constructs options based on the endpoint or something..

[august0715]

There's no way to do this in the same interface. You'd have to create multiple instances of feign, one for each deviation to Request.Options.

@adriancole
maybe,we could use an anotation on the method,such as:

@Retention(RUNTIME)
@Target({METHOD, TYPE})
public @interface ApiTimeout {

    int connectTimeoutMillis() default 10 * 1000;

    int readTimeoutMillis() default 60 * 1000;
}

public interface GitHub {

    @RequestLine("GET /repos/{owner}/{repo}/contributors")
    @ApiTimeout(connectTimeoutMillis = 20 * 10000)
    List<Contributor> contributors(@Param("owner") String owner, @Param("repo") String repo);

    @ApiTimeout(connectTimeoutMillis = 50 * 10000)
    List<Contributor> contributors1(@Param("owner") String owner, @Param("repo") String repo);
}

[sunnykaka]

We really need it.

[kdavisk6]

@tatyanayavkina

Thanks for taking the time and looking into this for us. My thoughts on this are that instead of creating a dedicated annotation, that we add these as optional values on the @RequestLine annotation instead. We can then extend it down to the other Contracts that extend Feign. What are your thoughts?

[tatyanayavkina]

@kdavisk6
Ok, I will move these options to @RequestLine

[rowi1de]

@kdavisk6, @tatyanayavkina I guess it is not the scope of this project, but a dedicated Annotation would make it easier for https://github.com/spring-cloud/spring-cloud-openfeign Users

[tatyanayavkina]

@kdavisk6 , @rowi1de

Should I

1. create new annotation @ApiTimeout or
2. add new fields to @RequestLine or
3. may be this feature does not need so much because the workaround exists?

[rowi1de]

Maybe have to take back my previous comment:
@RequestLine might be the best solution for this project.

But not sure how upstream compatibility is handled in this project.

org.springframework.cloud.openfeign.support.SpringMvcContract is using "@RequestMapping", which is the same Annotation for Controller (request-handling classes) mappings.
To add Client-Timeouts there would be misplaced, but that's an issue of https://github.com/spring-cloud/spring-cloud-openfeign and @ApiTimeout would also not work out of the box

[tatyanayavkina]

@kdavisk6,

I've reimplemented "per request timeout" via @RequestLine in separate branch (see commit https://github.com/tatyanayavkina/feign/commit/608a665b62d99103831e5e70680816c8eb36d825)
So you can compare both implementations.

As @rowi1de has noted, spring-cloud-openfeign will not get "per request timeout" feature out of the box anyway.
It is because spring-cloud-openfeign's SpringMvcContract does not use feign.Contract.Default as delegate contract.

Contracts that defined in open-feign use delegate contracts (except of JAXRSContract). And these contracts will support "per request timeout" only if delegate contract supports it.
And delegate contract can support "per request timeout" feature in both ways: via @RequestLine or via @ApiTimeout. As for JAXRSContract, this contract does not support @RequestLine annotation, so if we want to support "per request timeout" at this contract we should use one more annotation.

In my opinion, both implementations require the same code change size. But I think that @ApiTimeout implementation is better, because timeout settings and @RequestLine annotation settings are different kinds of settings, so I don't see causes to mix them in one annotation.
Also, code is more clear with @ApiTimeout.

What do you think?

[tatyanayavkina]

@kdavisk6

Or, we can use @ApiTimeout and parse it in ApiTimeoutParser(for example) not in feign.Contract#parseAndValidateMetadata
Then, code in ParseHandlersByName#apply(https://github.com/OpenFeign/feign/blob/master/core/src/main/java/feign/ReflectiveFeign.java#L153) can be changed to

public Map<String, MethodHandler> apply(Target key) {
      List<MethodMetadata> metadata = contract.parseAndValidatateMetadata(key.type());
      Map<SomeMethodName,RequestOption> timeoutOptions = ApiTimeoutParser.parseOptions(key.type());
      Map<String, MethodHandler> result = new LinkedHashMap<String, MethodHandler>();
      for (MethodMetadata md : metadata) {
        ...
		
        Options requestOptions = [if (optionsPerMethod contains timeoutOptions for method) then use timeoutOptions else use options];
        result.put(md.configKey(),
            factory.create(key, md, buildTemplate, requestOptions, decoder, errorDecoder));
      }
      return result;
}

and no need to make changes in all Contracts to support timeout options.

[kdavisk6]

@tatyanayavkina

There is not much we can do about custom Contract implementations. That's the reason why the Contract is an interface, for extension. If your intent is add this support so it can be used by all extensions of Feign, an annotation via the Contract will not work. We will need to consider an option that is part of core but not tied to an annotation.

Another possible solution is to update MethodMetadata with an optionsIndex where we can recognize the presence of a Request.Options or derivative, reference in the method signature.

interface MyClient {
   @RequestLine("GET /data")
   public MyResponse getMyData(Request.Options options);
}

This could then be used by any other derivative contract, here is an example of how this could work with Spring Cloud

interface MyClient {
   @RequestMapping(path = "/data")
   public MyResponse getMyData(Request.Options options);
}

What do you think?

[tatyanayavkina]

I think I have not enough expertise to implement this feature correctly, so I closed my pr.

[kdavisk6]

@tatyanayavkina

No need to close the PR. I am only one voice here and after talking about it with a few others, your approach does work and meets the needs of those interested in this feature. If you are still interested in working on this, please feel free to re-open or submit a new PR for review.