gproc_pool using claim strategy appears to be broken about gproc HOT 6 CLOSED

If I understand it correctly, what happens is that some claim attempts in the test run end up returning false, since the pool is full. The test pool is of size 6, and the number of workers is 10, so one should perhaps not be too surprised.

Obviously, the test needs to be fixed. The question is how? Right now, if the pool is full, claim/2 simply returns false, and the test code is not prepared to deal with that. The claim function itself doesn't offer any kind of retry or wait options. Perhaps it should?

from gproc.

Retry logic is usually left up to the caller (at least it is in most other pooling implementations I've been playing with for my Erlang Factory talk on pools), in that they return false (or busy or something like that), and the caller then decides what it wants to do. But several also support a queue of incoming requests and will block if all resources are used. So you sort of have the option, you could support a "blocking" mode which waits (maybe with a timeout, although many do not support a timeout), and a "non-blocking" mode which works how it does now.

However, I'm not sure that's the issue, I tried this

 case gproc_pool:claim (?POOL_ID,
                        fun (K,Pid) ->
                          io:format ("have ~p : ~p do ~p~n",[K,Pid,N]),
                          pt_gproc_worker:do (Pid, N, Data)
                        end) of
   {true, Res} -> Res;
   false -> {error, busy}
 end.

However, this prints

have {n,l,[gproc_pool,pt_gproc_pool,1,{pt_gproc_pool,1}]} : 1 do 5

Shouldn't the 1 actually be a pid as per the documentation?

I tried modifying the gproc:select here
https://github.com/uwiger/gproc/blob/master/src/gproc_pool.erl#L431
to

    case gproc:select({l,n}, [{ {{n,l,[?MODULE,Pool,'_','_']}, '$1', 0}, [],
                                [{{ {element,1,'$_'}, '$1' }}]}], 1) of

but that only seemed to help slightly in that it still had the same error

** exception error: bad argument
     in function  gproc:set_value/2
        called as gproc:set_value({n,l,
                                     [gproc_pool,pt_gproc_pool,5,
                                      {pt_gproc_pool,5}]},
                                  0)
     in call from gproc_pool:try_claim/3 (src/gproc_pool.erl, line 462)
     in call from gproc_pool:claim_/2 (src/gproc_pool.erl, line 434)
     in call from pt_gproc_sup:do/2 (src/pt_gproc_sup.erl, line 20)

Looking at the code (and adding a bunch of io:format/2 calls), I think the issue might be in how gproc:set_value is determining the pid and then passing it to the gproc_lib:do_set_value/3. If I'm reading these correctly, it seems like gproc:set_value is a gen_server call, and it takes the Pid from the From part of the call which means its the pid of the caller, then that gets passed and deep in gproc_lib:do_set_value the current value is looked up in the gproc ets table and if it matches things work, otherwise it's a badarg. Since the pid in the ets table is the pid of the worker process it will never match the caller and so any call to claim would fail, unless it's from within the worker itself. That seems sort of wrong as all the other gproc_pool methods are from the process attempting to get in contact with a worker. So I'm either using it wrong or there's a crossed wire in there.

Hopefully that makes sense.

from gproc.

Thanks for the digging. I've found a few issues, and am getting closer to a working test. I'll publish a branch as soon as I can.

I really appreciate your effort. As (I really hope) I've mentioned before, gproc_pool was an experiment. You are really making critical contributions to making it product-worthy (which I hope it can be).

from gproc.

I have fixed claim/2 so that it now passes the pid, as the docs say.

from gproc.

Great, this now works as I would expect. I assume you'll want to do more testing for completeness but it's at least far enough I can now use it in my Erlang Factory talk (I'm doing a talk looking at different pooling techniques and libraries to try to provide some guidance for beginner/intermediate developers, gproc_pool is on the more complex end simply because it doesn't manage your processes like say poolboy or pooler, but is more of a way to pick the processes, management is left up to you). Anyway, claim/2 is closest to the checkin/checkout semantics that other pooling libraries use, so is best for comparison.

from gproc.

There certainly should be more tests, but I think I should go ahead and merge the changes, since it obviously didn't work right before. :)

One can of course argue the point of gproc even attempting this kind of functionality. I guess the underlying idea is one of gproc as a method of organizing your system, publishing 'names' and 'properties' in an index of process footprints and dependencies. Essentially, once accustomed to debugging systems via gproc, a point of using gproc_pool would be that the pool properties adhere to the same inspection rules.

Feel free to challenge the relevance of that argument to process pools. ;-)

from gproc.