returning worker to pool too early?

[lefant]

Hi,

I was just reading through your code (planning to use poolboy for a connection pool myself), when I noticed something weird in eredis_pool:q/3:

q(PoolName, Command, Timeout) ->
Worker = poolboy:checkout(PoolName),
poolboy:checkin(PoolName, Worker),
Reply = eredis:q(Worker, Command, Timeout),
Reply.

shouldn't the poolboy:checkin.. really happen after the Worker has done its eredis:q... request? like so:

q(PoolName, Command, Timeout) ->
Worker = poolboy:checkout(PoolName),
Reply = eredis:q(Worker, Command, Timeout),
poolboy:checkin(PoolName, Worker),
Reply.

otherwise the worker immediately becomes available to other concurrent clients, even before it is done with the request, no?

I am not really planning to use redis at the moment, so ignore / delete at your leisure, but since I already spotted it, I figured I may as well drop you a note.

Fabian

[hiroeorz]

Sorry I didnt notice your message... And thanks for your message :-)

At first, I am not measuring actual speed.

////////////////
q(PoolName, Command, Timeout) ->
Worker = poolboy:checkout(PoolName),
Reply = eredis:q(Worker, Command, Timeout),
poolboy:checkin(PoolName, Worker),
Reply.
///////////////

I think that yes, the method is usually right.
And I am trying to do similarly.

But I read source code of eredis and I notice that eredis dont block receive next request, until returning result.
https://github.com/wooga/eredis/blob/master/src/eredis_client.erl

And I read document for redis, and I got to know that redis suppot pipelining server-client protocol.
http://redis.io/topics/pipelining

I think... [checkout -> eredis:q(...) -> checkin] will be repealed pipelining protocol.

And poolboy not necessary for eredis....? No. redis can receive request non-blocking but order of response is same as request order.
If big data is stored. Another response is kept waiting. So I use poolboy by a different method from usual.

And I readed source code of poolboy. Poolboy using queue:in and queue:out for management of process pool. queue:in add value to rear of queue, and queue:out removes from front of queue. So I thought that the same process did not receive a request repeatedly.
https://github.com/devinus/poolboy/blob/master/src/poolboy.erl

Now, I think [checkout -> checkin -> eredis:q(...)] logick is best method for using poolboy for eredis utilize pipelining.

Thank you :-)

[devinus]

@hiroeorz It's true, you should never checkin the worker before you're done working with it. I'm unfamiliar with how eredis works, but you shouldn't rely on an internal implementation detail of Poolboy as that may change any day. I even recommend using the new poolboy:transaction/1 method. You can see an example here: https://github.com/devinus/epgsql_pool/blob/master/src/epgsql_pool.erl#L11

[hiroeorz]

I understand. I changed to using poolboy:transaction/2. Thank you :-)