fix Redis connection sharing

[Ladicek]

When a single RedisConnection is shared among multiple contexts (e.g. verticles), pipelining doesn't work correctly, because there is no coordination for concurrent access. This commit fixes that by making sure that all requests are performed from the same context on which the NetSocket was created. If the current context is different from the NetSocket context, the request is emitted on the correct context, which means no concurrent access, all requests are serialized.

Fixes #394

[Ladicek]

@vietj I believe this implements what we discussed earlier today. I'd really like your review on this :-)

When done, I will backport the fix to 4.x.

[Ladicek]

Humm. Will investigate the failure next week.

[Ladicek]

Turns out when I moved from NetClient.connect(SocketAddress) to NetClientInternal.connectInternal(ConnectOptions, Promise, ContextInternal), I didn't create the ConnectOptions fully -- I also have to pass the SSL options.

Fixed.

[Ladicek]

One more point to discuss maybe, @vietj

I recognize this

@Override
public Future<Response> send(final Request request) {
  if (context == vertx.getContext()) {
    return doSend(request);
  } else {
    Promise<Response> promise = vertx.getOrCreateContext().promise();
    context.runOnContext(ignored -> {
      try {
        doSend(request).onComplete(promise);
      } catch (Exception e) {
        promise.fail(e);
      }
    });
    return promise.future();
  }
}

is fairly low-level. If I understand correctly, this

@Override
public Future<Response> send(final Request request) {
  Promise<Response> promise = vertx.getOrCreateContext().promise();
  context.emit(ignored -> {
    try {
      doSend(request).onComplete(promise);
    } catch (Exception e) {
      promise.fail(e);
    }
  });
  return promise.future();
}

would be a shorter equivalent, except that it always allocates an extra Promise/Future, even if the current context is the same as the original context. Which variant would be considered more idiomatic?

[Ladicek]

I decided to backport to 4.x proactively, here goes: #424

[vietj]

One more point to discuss maybe, @vietj

I recognize this

@Override
public Future<Response> send(final Request request) {
  if (context == vertx.getContext()) {
    return doSend(request);
  } else {
    Promise<Response> promise = vertx.getOrCreateContext().promise();
    context.runOnContext(ignored -> {
      try {
        doSend(request).onComplete(promise);
      } catch (Exception e) {
        promise.fail(e);
      }
    });
    return promise.future();
  }
}

is fairly low-level. If I understand correctly, this

@Override
public Future<Response> send(final Request request) {
  Promise<Response> promise = vertx.getOrCreateContext().promise();
  context.emit(ignored -> {
    try {
      doSend(request).onComplete(promise);
    } catch (Exception e) {
      promise.fail(e);
    }
  });
  return promise.future();
}

would be a shorter equivalent, except that it always allocates an extra Promise/Future, even if the current context is the same as the original context. Which variant would be considered more idiomatic?

where is the extra promise allocated ?

[Ladicek]

I rewrote to your suggestion with execute(), which is shortest and doesn't allocate any extra Promise, so I think it's best :-)

[vietj]

I rewrote to your suggestion with execute(), which is shortest and doesn't allocate any extra Promise, so I think it's best :-)

ah right out of order messages :-)