Comments (15)
The bug is more complex than I thought at first. If the following line throws a :timeout
:
Line 201 in 67847f1
The caller process won't die because we have the catch. If the caller process doesn't die later (maybe it is a long live process), caller process will finally receive the message from the pool but won't handle it. This means the pool thinks the worker is busy because the caller process is still alive, but the caller process is not using it. I hope I have made myself clear.
I checked poolboy and it handles this situation. Instead of using a direct Genserver.call, it uses the checkout function. In this function, poolboy sends a cancel message if it times out. I think this is the right solution:
https://github.com/devinus/poolboy/blob/master/src/poolboy.erl#L60-L67
from poolex.
I pushed a test to prove the second bug: alexcastano@29baf0a#diff-c656132eac013486857824dcb3601cf0f0d676b6e82d976328f9c862d2e37df0R275-R278
from poolex.
Hi, Alex!
I will check. Thanks for the issue 🙏🏻
from poolex.
Thanks again for describing the problem! I fully agree that there needs to be more information on the call site.
Help, please, to clarify the idea with the common :timeout
. Do I understand correctly that we are talking about a timeout determining the total time to complete the task (time to get the worker from the queue + time to execute the function)?
If I understand correctly, the idea sounds sensible. If we treat the pool in the same way as we treat the database, and it is not important for us to define timeouts for different stages, but we need to limit the time of tasks, then the total timeout is what we need :)
from poolex.
I also agree about the second bug, but fixing it after (or together with) changing the timeout logic might be more correct.
from poolex.
Sorry, I mixed up many issues in this ticket.
I looked at the code, and now I have more insights. Let me explain in part:
- The current
:timeout
param was confusing us because we expected a different behaviour, more similar to the Ecto or GenServer. However, you are right. Because the function is executing in the caller process, implementing:timeout
could be difficult and misleading. In my opinion, we should just change:timeout
option to:checkout_timeout
to avoid confusion. - I would implement the
checkin
andcheckout
functions in a similar way to the:poolboy
does. I would also make them public, it makes sense to me, but it is up to you. That would fix both bugs at once. - I'm unsure if the function
run/1
should capture errors from the given function. After all, it will run in the same caller process. It should be up to the user to decide if they want to capture or not the errors. - Similarly, we should standardize the replies in the
run/1
function. It should return always{:ok, result}
or{:error, type_of_error}
. When we return{:ok, _}
, the given function could be executed successfully. When we return{:error, _}
, it could not be executed for any reason. These reasons are:
4a.{:error, :checkout_timeout}
.
4b.{:error, {:runtime_error, error}}
if you still want to return the runtime errors. I wouldn't do it.
If you don't have time to fix it, I will try to make a PR because we need this fixed too :)
from poolex.
In my opinion, we should just change :timeout option to :checkout_timeout to avoid confusion.
I agree with this idea. Separate checkout timeouts and runtime timeouts sound right :)
In the future, I would also consider the general timeout. This may be more difficult to implement, but on the other hand, it can greatly simplify understanding of the use.
I would also make them public, it makes sense to me, but it is up to you.
To be honest, I don't know how it would be better. I felt there is no real reason for using such interfaces and reducing it to a single and understandable universal run
is better. Please tell me what approaches you see in using checkin
and checkout
when working with a pool.
if you still want to return the runtime errors. I wouldn't do it.
Could you tell me what you propose to return if a runtime error happens? If there are two interfaces, run
and run!
, we must somehow handle the error in the case of run
.
from poolex.
If you don't have time to fix it, I will try to make a PR because we need this fixed too :)
I suggest implementing this within 1 or 2 weeks. But if you need faster, then I would be glad to see a PR from your side 🙏🏻
from poolex.
In my opinion, we should just change :timeout option to :checkout_timeout to avoid confusion.
I agree with this idea. Separate checkout timeouts and runtime timeouts sound right :) In the future, I would also consider the general timeout. This may be more difficult to implement, but on the other hand, it can greatly simplify understanding of the use.
I don't see a straightforward implementation of the general :timeout
. So, if it is not a mandatory feature I would prefer not to overcomplicate the code.
I would also make them public, it makes sense to me, but it is up to you.
To be honest, I don't know how it would be better. I felt there is no real reason for using such interfaces and reducing it to a single and understandable universal
run
is better. Please tell me what approaches you see in usingcheckin
andcheckout
when working with a pool.
I won't use it for my current case, but I can imagine a situation where a GenServer wants to have more control over the resource. It is riskier, but it is up to the user to use.
if you still want to return the runtime errors. I wouldn't do it.
Could you tell me what you propose to return if a runtime error happens? If there are two interfaces,
run
andrun!
, we must somehow handle the error in the case ofrun
.
I understand returning errors when the code is executed in another process, and I would like to know what happened. However, the given function runs precisely in the same process. If the user really has to handle runtime errors, it is possible to do it in the function:
Poolex.run(:my_pool, fn server ->
try do
whatever(server)
rescue
... -> {:runtime_error, :whatever_failed}
end
end)
Or even outside of the function:
try do
Poolex.run(:my_pool, &whatever/1)
catch
_ -> :whatever
end
This approach (user handles the runtime errors) for this library makes more sense to me, but it is not a strong opinion.
from poolex.
I checked the code. This also means we should constantly monitor the caller, not only when waiting for a worker, to release the worker in case of a crash. Similar thing for checkin & checkout functions.
from poolex.
I won't use it for my current case, but I can imagine a situation where a GenServer wants to have more control over the resource. It is riskier, but it is up to the user to use.
It may be worth leaving these functions private. And make them public only when we are sure it is needed.
I understand returning errors when the code is executed in another process, and I would like to know what happened. However, the given function runs precisely in the same process. If the user really has to handle runtime errors, it is possible to do it in the function...
If we define a function without a bang, it must comply with the agreement and not raise runtime errors. But you're right; this is not the necessary complication, and control should be transferred to the call site. It's currently challenging to comprehend the correct way of doing it.
from poolex.
Hi, @alexcastano!
This also means we should constantly monitor the caller, not only when waiting for a worker, to release the worker in case of a crash.
I want to inform you that this trouble was fixed at #56.
from poolex.
Could you please check the latest release? What do you think, the main problem was solved? Do we still need checkin
and checkout
functions?
from poolex.
Oh, I see. I have rechecked your test cases, and the problem with timeout still needs to be solved.
from poolex.
Hi! Sorry for the delay!
Could you please check the latest release? What do you think, the main problem was solved? Do we still need
checkin
andcheckout
functions?
You need the functions if you want to use the library in a Genserver. Suppose your Genserver and the resource from the pool have to send several messages between them because it uses an async interface. Your only option is to allow the Genserver to use checkin
and checkout
to handle the resource.
I tried to implement it, but it was a significant change because now you must also monitor the clients. I didn't have enough time, so I moved back to :poolboy
for the moment. This is my branch: develop...alexcastano:poolex:cancel_checkout
IMO, the safer step would be to copy how :poolboy
works. It has some minor bugs like "async shutdown", but it's battle-tested.
I'm going to check your changes now and I will tell you what I think. Please take it as just another opinion. I intend to help you create a stable library that many Elixir users can use in the future.
from poolex.
Related Issues (15)
- Support timeout option on `Poolex.run`
- Delivery to hex.pm on releases HOT 1
- Ability to use different implementations for process lists and queues
- Using `Stream.resource` to safe wrap getting a free worker HOT 1
- Split interface into `run/3` and `run!/3`
- `Overflow` feature
- Provide `child_spec/2` function HOT 1
- Get rid of `Application` configuration for worker and caller implementations
- Warning on docs generating
- Shutdown process HOT 4
- Poolex doesn't verify if there are callers after a worker dies HOT 1
- Capture crash logs in tests
- Ways to interact with pools as units. HOT 2
- Grace period before idle workers are stopped HOT 2
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from poolex.