Fix sync/async/reactive methods for thread-safe #302

Closed
wants to merge 1 commit into
from

Projects

None yet

3 participants

@jongyeol
Contributor
jongyeol commented Jul 5, 2016 edited

I found bugs related with thread safety in *ConnectionImpls. So, I fixed sync/async/reactive methods of *ConnectionImpls' using double checked locking.

Refers:

Make sure that:

  • You have read the contribution guidelines.

  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.

  • You submit test cases (unit or integration tests) that back your changes.

@coveralls
coveralls commented Jul 5, 2016 edited

Coverage Status

Coverage increased (+0.4%) to 93.433% when pulling 130457e on jongyeol:feature/connection-impl-thread-safe into 0af87eb on mp911de:master.

@jongyeol
Contributor
jongyeol commented Jul 5, 2016

@mp911de , There are no netty-41next profile in pom.xml and using ssl were failed in CI. But, make test was passed in local (mac os x). I think this test failures are not related with this PR.
So, Anyway, Could you review this PR?

@mp911de
Owner
mp911de commented Jul 5, 2016

Thanks for the PR. I fixed the profile in the TravisCI config. I think we could even perform eager initialization of the API wrapper objects and making these final. API wrappers are lightweight objects, they even though they construct a command builder.

@jongyeol
Contributor
jongyeol commented Jul 6, 2016

@mp911de , I think you're worrying about volatile's performance decrement. Isn't it? Then, did you mean this style?

(via https://en.wikipedia.org/wiki/Double-checked_locking#Usage_in_Java)

public class FinalWrapper<T> {
    public final T value;
    public FinalWrapper(T value) {
        this.value = value;
    }
}

public class Foo {
   private FinalWrapper<Helper> helperWrapper;

   public Helper getHelper() {
      FinalWrapper<Helper> wrapper = helperWrapper;

      if (wrapper == null) {
          synchronized(this) {
              if (helperWrapper == null) {
                  helperWrapper = new FinalWrapper<Helper>(new Helper());
              }
              wrapper = helperWrapper;
          }
      }
      return wrapper.value;
   }
}

In this, if we use final for helperWrapper with eager initialization like follow codes. Then, public final T value must be volatile like AtomicReference's implementation. Or, CPUs' L1,L2 will be different.
(refer: https://github.com/openjdk-mirror/jdk7u-jdk/blob/f4d80957e89a19a29bb9f9807d2a28351ed7f7df/src/share/classes/java/util/concurrent/atomic/AtomicReference.java#L60)

public class FinalWrapper<T> {
    private volatile T value; // this must be volatile in this case.
    public FinalWrapper(T value) { this.value = value; }
    public void set(T value) { this.value = value; }
    public T get() { return value; }
}

public class Foo {
   private final FinalWrapper<Helper> helperWrapper = new FinalWrapper<>(null);
   ...
}

I think we cannot use final for API wrapper instance and avoiding volatile together. If you didn't mean this, could you explain about it more?

@mp911de
Owner
mp911de commented Jul 6, 2016

Thanks for your detailed response. I'm not worried about volatile. The code here isn't on the critical path, sync()/async()/reactive() isn't something you usually call on each command invocation.
There are other places (like CommandHandler) that make heavy use of volatile and that maybe could need optimization as CommandHandler is involved in each command invocation.

Seeing all that effort about synchronization and lazy initialization make me think whether lazy initialization is really needed and I came to the conclusion, that it's not. Therefore, I'd perform the initialization right in the constructor.

@jongyeol
Contributor
jongyeol commented Jul 6, 2016

I see. Well then, may I change sync/async/reactive to eager initialization in constructors?

@jongyeol jongyeol Fix sync/async/reactive methods for thread-safe
38414a0
@jongyeol
Contributor
jongyeol commented Jul 6, 2016

@mp911de , I updated this PR with eager initialization of sync/async/reactive. Could you review this?

@coveralls
coveralls commented Jul 6, 2016 edited

Coverage Status

Coverage increased (+0.2%) to 93.306% when pulling 38414a0 on jongyeol:feature/connection-impl-thread-safe into 1727b06 on mp911de:master.

@mp911de
Owner
mp911de commented Jul 6, 2016

Thanks a lot. It looks good, going to merge the PR in the next days.

@mp911de mp911de added the enhancement label Jul 6, 2016
@mp911de mp911de added a commit that referenced this pull request Jul 7, 2016
@mp911de Polishing #302 b655300
@mp911de mp911de added a commit that referenced this pull request Jul 7, 2016
@mp911de Polishing #302 fd7dcc6
@mp911de
Owner
mp911de commented Jul 7, 2016

That's merged and polished with fd7dcc6.

@mp911de mp911de closed this Jul 7, 2016
@mp911de mp911de added this to the Lettuce 4.3.0 milestone Jul 7, 2016
@jongyeol jongyeol deleted the jongyeol:feature/connection-impl-thread-safe branch Jul 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment