jurmous / etcd4j

Java / Netty client for etcd, the highly-available key value store for shared configuration and service discovery.
Apache License 2.0
267 stars 83 forks source link

may it cause a concurrent problem while using ResponsePromise.addListene method? #187

Open mashirofang opened 5 years ago

mashirofang commented 5 years ago

hello! I was planning to use it's async way in my program, and I saw the code in ResponsePromise like this

/**
   * Add a promise to do when Response comes in
   *
   * @param listener to add
   */
  public void addListener(IsSimplePromiseResponseHandler<T> listener) {
    if (handlers == null) {
      handlers = new LinkedList<>();
    }

    handlers.add(listener);

    if (response != null || exception != null) {
      listener.onResponse(this);
    }
  }

  /**
   * Handle the promise
   *
   * @param promise to handle
   */
  protected void handlePromise(Promise<T> promise) {
    if (!promise.isSuccess()) {
      this.setException(promise.cause());
    } else {
      this.response = promise.getNow();
      if (handlers != null) {
        for (IsSimplePromiseResponseHandler<T> h : handlers) {
          h.onResponse(this);
        }
      }
    }
  }

Since it use an un-concurrent-safe "LinkedList" and do not has a lock , would I take the risk that my handler would invoke more than once or somewhere throws an concurrent exception?

lburgazzoli commented 5 years ago

yep it could, do you mind sending a pr to fix it ?

mashirofang commented 5 years ago

Of course ,I'm glad to do that

mashirofang commented 5 years ago

yep it could, do you mind sending a pr to fix it ?

done with a simple lock