search

dashbitco / nimble_pool

A tiny resource-pool implementation for Elixir
347 stars 20 forks source link

feat: implement terminate_pool/2 callback #38

Closed oliveigah closed 1 year ago

oliveigah commented 1 year ago

As mentioned on https://github.com/dashbitco/nimble_pool/issues/37, in order to implement pool telemetry features on nimble_pool clients would be nice to have some callback regarding pool termination.

The implementation is very straight forward but I'm not sure if the changes that I made to the tests are the best approach.

I had to always accept terminate_pool as a valid instruction.

The stateless is tricky, because this callback does not have access to worker state and therefore I need to work around this by introspecting into pool_state it self and if more complex use cases must be tested we gonna need another work around to update the remaining instructions.

The stateful implementation I think is ok, because terminate_pool is valid only when there is no more instructions and also there is no need for a workaround on the worker state.

Let me know what you think! Thanks in advance! :smiley:

josevalim commented 1 year ago

For the tests, you can probably create a separate pool module or a separate worker. Just start the pool with a single worker, and then message the parent. Verify that terminate_worker is called before terminate_pool and that's it. :)

oliveigah commented 1 year ago

For the tests, you can probably create a separate pool module or a separate worker. Just start the pool with a single worker, and then message the parent. Verify that terminate_worker is called before terminate_pool and that's it. :)

Great! So I think the stateful pool test is enough! I removed the terminate_pool callback from the stateless one since it may hide some bugs due to the test implementation.

Let me know what you think!

josevalim commented 1 year ago

:green_heart: :blue_heart: :purple_heart: :yellow_heart: :heart: