-
Notifications
You must be signed in to change notification settings - Fork 54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix the context passed to the driver factory #254
Conversation
Can you add a bit more…uh…context, to the commit description? I ask because this change looks structurally reasonable, but I think we need to be careful not to use a request-specific context for pool maintenance. This is a long-running task that is not request-specific, and is one of the unusual cases where it might make sense for the pool to capture and hold a Context value for a long time. Most of the time that will be the background context, but we do want the main package to be able to control shutdown, so I wouldn't hard-wire it to that. That said: I'm not sure that in this case we actually want to plumb the context through the request flow—it might actually be simpler (or perhaps not) to store it at construction. |
@creachadair I agree that passing the context directly is not an ideal solution. We definitely need to rewrite the pool to manage instances on a separate goroutine, separately from user requests. But it will require more work and time, while I think this issue blocks one of the PoCs to a certain degree. The change should work reasonably well - it will create driver instance on the user request, as it was doing before, but it will allow canceling the creation if the user sets a short deadline. But yes, we will waste time to start and kill the driver. Looking at the code, it requires significantly more effort to rewrite it to be separate. I'm working on it right now. |
I agree, and I don't think we should block this PR for that. But if you're willing to add some more of the details—of why we're doing this, now, and what will need to be done to make it better—in the PR description and possibly as comments, that would help.
Yeah, it is definitely a bigger project, and I don't see any problems with this solution for now. I'm mainly just asking for more docs. :) |
Previous changes added a context argument to newDriverPool, which is called when the first request for the driver is received. By mistake, the context of this first request is captured by the closure and is used to create all other driver instances. Thus, if the first request specifies the context timeout and fails for some reason, all future requests will fails with "context cancelled" error. Instead, we need to pass the real context from the user request down to the pool's driver factory function. This is not an ideal way of managing drivers - pool should be managed by a separate goroutine. This will be implemented later. This is merely a hotfix for the real issue. Signed-off-by: Denys Smirnov <denys@sourced.tech>
6babafa
to
a6677e0
Compare
Updated the commit message and the description. Ready for another round :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the clarification!
I guess with this solution we are still tying a process lifetime to a request lifetime, but that's better than having them all die. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏
and thank you for clear explanation in the commit msg!
Previous changes added a context argument to newDriverPool, which is called when the first request for the driver is received. By mistake, the context of this first request is captured by the closure and is used to create all other driver instances. Thus, if the first request specifies the context timeout and fails for some reason, all future requests will fail with "context canceled" error.
Instead, we need to pass the real context from the user request down to the pool's driver factory function.
This is not an ideal way of managing drivers - pool should be managed by a separate goroutine. This will be implemented later. This is merely a hotfix for the real issue.
Fixes #253
Signed-off-by: Denys Smirnov denys@sourced.tech