OpenFeign / feign

Feign makes writing java http clients easier
Apache License 2.0
9.43k stars 1.92k forks source link

Feign.Builder have a requestInterceptor bug #2249

Closed hdmvp closed 1 day ago

hdmvp commented 9 months ago

for example: public static T getHttpApi(Class api, String url) { ApplicationContext applicationContext = SpringContext.getApplicationContext(); return applicationContext.getBean(Feign.Builder.class).requestInterceptor(new CustomFeignInterceptor()).target(api, url); } add the requestInterceptor at here cause :

java.util.ConcurrentModificationException

at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1631)....

suggest:

 /**
     * Adds a single request interceptor to the builder.
     */
    public Builder requestInterceptor(RequestInterceptor requestInterceptor) {
      this.requestInterceptors.add(requestInterceptor);
      return this;
    }
 private final List<RequestInterceptor> requestInterceptors =
        new ArrayList<RequestInterceptor>();

the soureCode requestInterceptors should be a CopyOnWriteArrayList

lquterqtd commented 9 months ago

requestInterceptor只需要add一次就可以吧? 你这里貌似是并发添加了,可以控制一下调用的地方,只add一次

hdmvp commented 9 months ago

requestInterceptor只需要add一次就可以吧? 你这里貌似是并发添加了,可以控制一下调用的地方,只add一次

这里每次的调用确实会存在并发添加的问题,但是基于暴露的方法来看,如果不点进源码去看,根本就不会发现这个问题。所以基于一个良好的体验来讲,我觉得这个requestInterceptor方法的内部集合实现有改进的空间,换成 CopyOnWriteArraySet会更友好一点

kdavisk6 commented 1 day ago

From the pull request, our builders are not intended to be thread-safe. So this is expected behavior.