Add support for redigo's Pool.GetContext (to support waiting with timeout when pool is full)

[iwanbk]

Default behaviour of redigo Pool is to simply returns invalid connection
when the pool is full/exhausted (no available connection).

The problem is described in more details at gomodule/redigo#56

We could add a kind of blocking with timeout when getting connection from redigo pool.
So, instead of simply returns invalid connection, we could wait first for a configurable amount of time.

It could be implemented using sempahore (from https://godoc.org/golang.org/x/sync/semaphore).

I have some implementation at tokopedia@2fa113e.

If you have interest, i could create a proper PR with proper tests

[mna]

Hello,

This package only deals with redis cluster, all of the standard redis features (including connections) are handled by the redigo package that redisc uses / wraps. As I understand it, Gary wasn't against the idea of adding this feature, he mentions in the issue you linked that he would accept such a PR. He is not the maintainer anymore, but possibly the new one(s) would accept such a PR too, this should be discussed over on the redigo repo.

Thanks,
Martin

[iwanbk]

Hello,

I send it here instead of to redigo repo because of these reasons:

• it doesn't modify redigo at all, it only wrap it with semaphore
• to make it work on redigo, it will change it's API.
• it is just simpler to make the wrapper rather than change the redigo

But of course it is your call :)

[mna]

It doesn't modify redigo but it modifies redisc, while the change belongs in redigo. Looking at the latest redigo API, I think it actually supports this already, with a combination of setting Pool.Wait = true and using Pool.GetContext to get a connection. What would be missing in redisc is to support the GetContext variant of obtaining a connection.

[iwanbk]

Looking at the latest redigo API, I think it actually supports this already, with a combination of setting Pool.Wait = true and using Pool.GetContext to get a connection.

Ah, i really didn't aware about it

What would be missing in redisc is to support the GetContext variant of obtaining a connection.

It seems you are open to this, right?

Then i'll be more than happy to work on it.

[mna]

It seems you are open to this, right?

Then i'll be more than happy to work on it.

Definitely, thanks for looking into this. Just make sure the Cluster.GetContext signature is the same as Pool.Get (to make it easy to switch Pool for Cluster in codebases) and to add tests, I'll be happy to review and assist in getting this merged. I'll reopen the ticket with an updated title that reflects this.

[iwanbk]

Hi @mna

Just make sure the Cluster.GetContext signature is the same as Pool.Get

I tried to add Cluster.GetContext(ctx), but i found that the code change to existing code will be too much, or worse we have to restructure the code, which is i'm not familiar with.

The reason is that we have to keep the passed context.Context.

I propose to do this:

• add field to Cluster, maybe number of millisecond or time.Duration to wait
• modify

  redisc/cluster.go

  Lines 237 to 239 in fe71609

  	conn := p.Get()
  	return conn, conn.Err()

call pool.GetContext if not zero, with context.WithTimeout.

to make it easy to switch Pool for Cluster in codebases)

Unfortunately, my proposal couldn't satisfy this requirement.

[mna]

Hey, thanks for the update! Yeah I understand that it's not obvious to change a codebase you're not familiar with. Your proposal makes sense, but that's only one of the benefits of using context.Context, with a static timeout, but consider if getting the cluster connection was part of a larger job controlled by a context, the caller might be interested to stop it using a cancel function. I'd like to be able to support the GetContext as it is, if possible.

I'll try to take a closer look later this week. If it fails then your proposal is definitely a plan B to consider!

[mna]

@iwanbk I started work on GetContext support in the wip-getcontext branch, you can try out it if you want, note that I still need to add tests to it: https://github.com/mna/redisc/compare/wip-getcontext?expand=1

I will also have to document the very narrow use of the context that redigo makes, it is really only used when waiting for an active connection to be available (i.e. it cannot stop a dial in progress, as net.DialContext is not used). Also, it doesn't make much sense to reuse a context in a RetryConn, it is only used for the initial connection, never for a redirection (it would likely fail anyway in many cases, e.g. if the caller uses a context with a short timeout to wait for the initial connection, a redirection may be required much later and the context will be expired).

I'm not 100% convinced this makes much sense to support because of this (such a narrow use-case). But anyway, the initial support is there, let me know how it works for you and I'll keep thinking about it.

[iwanbk]

it is really only used when waiting for an active connection to be available (i.e. it cannot stop a dial in progress, as net.DialContext is not used)

Yes, true

Also, it doesn't make much sense to reuse a context in a RetryConn, it is only used for the initial connection, never for a redirection

Yes, agree

From above statement, did you also imply that

redisc/cluster.go

Line 245 in d3c4b20

return poolGet(ctx, p)

is only called in initial connection? Never in RetryConn?

I'm not 100% convinced this makes much sense to support because of this (such a narrow use-case). But anyway, the initial support is there, let me know how it works for you

I 100% agree with your concern and i feel the same

I'll keep thinking about it.

i'll do the same

[mna]

From above statement, did you also imply that
return poolGet(ctx, p)
is only called in initial connection? Never in RetryConn?

No, it is called whenever a (pooled) connection needs to be established, but in RetryConn the ctx will always be nil (i.e. unused).

[mna]

I think your proposal may make more sense than adding GetContext, after all. Given that the context serves only that specific purpose of waiting on a pooled connection, having a field on the Cluster named maybe PoolWaitTimeout time.Duration and then passing a context with this timeout if it is set > 0. If you're still interested in working on this, I'd let you prepare a PR for it - only gotcha is that it must check for the Go version to support the Pool.GetContext call (I think it's fine to have the field on the Cluster regardless of the Go version, but it is only used if Go > 1.7).

[iwanbk]

If you're still interested in working on this, I'd let you prepare a PR for it

sure, will do.

[iwanbk]

I created the initial implementation at https://github.com/mna/redisc/compare/master...iwanbk:add-get-timeout?expand=1

Is it OK for me to add dependency to miniredis for the test.
So, i will test the getFromPool func in isolation.

[mna]

Is it OK for me to add dependency to miniredis for the test.

I'd rather not, the existing testing helpers should already allow testing this, you can get inspiration from how they tested it in redigo, only thing is I don't think they covered the case where the wait allows getting a conn (i.e. an active conn got closed during the wait time), we should cover that.

Thanks for tackling this, much appreciated!

[mna]

Also, one small thing, you should use context.Background() as base context instead of context.TODO().

[iwanbk]

I'd rather not, the existing testing helpers should already allow testing this

OK, i wrote some initial code using redistest.StartMockServer, i think this is the best testhelper for this func

only thing is I don't think they covered the case where the wait allows getting a conn (i.e. an active conn got closed during the wait time), we should cover that.

sure

Also, one small thing, you should use context.Background() as base context instead of context.TODO()

oh my bad

[mna]

Hey, thanks, I just took a quick look, your changes look great! Only thing is the test file should have the build go1.7 tag on it too, otherwise it would fail on go < 1.7. Feel free to open a PR whenever you're ready to get this merged!