Skip to content
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(cmd): stop generators as last to avoid deadlock on exit #368

Closed
wants to merge 2 commits into from

Conversation

nuivall
Copy link
Member

@nuivall nuivall commented Jun 22, 2023

As jobs unconditionally read from generator's channel there can be
a situation when generator is closed and will not produce
any new values while some jobs still wait on empty channel deadlocking
and preventing process exit.

@dkropachev
Copy link
Collaborator

dkropachev commented Jun 22, 2023

As jobs unconditionally read from generator's channel there can be a situation when generator is closed and will not produce any new values while some jobs still wait on empty channel deadlocking and preventing process exit.

Thanks for PR, it is better to make .Get return nil when context is Canceled. Please take a look at #372

@nuivall
Copy link
Member Author

nuivall commented Jun 23, 2023

As jobs unconditionally read from generator's channel there can be a situation when generator is closed and will not produce any new values while some jobs still wait on empty channel deadlocking and preventing process exit.

Thanks for PR, it is better to make .Get return nil when context is Canceled. Please take a look at #372

Why is it better? You're adding select cost to each request sent.

@dkropachev
Copy link
Collaborator

As jobs unconditionally read from generator's channel there can be a situation when generator is closed and will not produce any new values while some jobs still wait on empty channel deadlocking and preventing process exit.

Thanks for PR, it is better to make .Get return nil when context is Canceled. Please take a look at #372

Why is it better? You're adding select cost to each request sent.

It is better because it does not create more intertwining between contexts as result you do not care when to close generators context.
But your idea is even better - we need to drop context from the partition and close channels instead when generator cycle is ended.

@nuivall
Copy link
Member Author

nuivall commented Jun 23, 2023

As jobs unconditionally read from generator's channel there can be a situation when generator is closed and will not produce any new values while some jobs still wait on empty channel deadlocking and preventing process exit.

Thanks for PR, it is better to make .Get return nil when context is Canceled. Please take a look at #372

Why is it better? You're adding select cost to each request sent.

It is better because it does not create more intertwining between contexts as result you do not care when to close generators context. But your idea is even better - we need to drop context from the partition and close channels instead when generator cycle is ended.

Technically they have no relation to each other because those are 2 different contexts - not connected. I agree that doing some dance around closing channels could be better in principle but it is much more risky patch - as per sending to closed channel case, and it'd introduce separate closing mechanism to already existing one which would be confusing.

I think it's common pattern that dependency is created first and destructed last, which is the case here. Not ideal though as it's a weird pattern that generator has it's own waitgroup which nobody waits for.

As jobs unconditionally read from generator's channel there can be
a situation when generator is closed and will not produce
any new values while some jobs still wait on empty channel deadlocking
and preventing process exit.
@dkropachev
Copy link
Collaborator

As jobs unconditionally read from generator's channel there can be a situation when generator is closed and will not produce any new values while some jobs still wait on empty channel deadlocking and preventing process exit.

Thanks for PR, it is better to make .Get return nil when context is Canceled. Please take a look at #372

Why is it better? You're adding select cost to each request sent.

It is better because it does not create more intertwining between contexts as result you do not care when to close generators context. But your idea is even better - we need to drop context from the partition and close channels instead when generator cycle is ended.

Technically they have no relation to each other because those are 2 different contexts - not connected. I agree that doing some dance around closing channels could be better in principle but it is much more risky patch - as per sending to closed channel case, and it'd introduce separate closing mechanism to already existing one which would be confusing.

I think it's common pattern that dependency is created first and destructed last, which is the case here. Not ideal though as it's a weird pattern that generator has it's own waitgroup which nobody waits for.

I would rather prefer to make it straigh than further entangle logic even if it requires more effort.

@nuivall
Copy link
Member Author

nuivall commented Jun 23, 2023

As jobs unconditionally read from generator's channel there can be a situation when generator is closed and will not produce any new values while some jobs still wait on empty channel deadlocking and preventing process exit.

Thanks for PR, it is better to make .Get return nil when context is Canceled. Please take a look at #372

Why is it better? You're adding select cost to each request sent.

It is better because it does not create more intertwining between contexts as result you do not care when to close generators context. But your idea is even better - we need to drop context from the partition and close channels instead when generator cycle is ended.

Technically they have no relation to each other because those are 2 different contexts - not connected. I agree that doing some dance around closing channels could be better in principle but it is much more risky patch - as per sending to closed channel case, and it'd introduce separate closing mechanism to already existing one which would be confusing.
I think it's common pattern that dependency is created first and destructed last, which is the case here. Not ideal though as it's a weird pattern that generator has it's own waitgroup which nobody waits for.

I would rather prefer to make it straigh than further entangle logic even if it requires more effort.

That'd be best, but ending some things by context cancellation and some things by channel closing or default select I'd not say it's straight.

@dkropachev
Copy link
Collaborator

As jobs unconditionally read from generator's channel there can be a situation when generator is closed and will not produce any new values while some jobs still wait on empty channel deadlocking and preventing process exit.

Thanks for PR, it is better to make .Get return nil when context is Canceled. Please take a look at #372

Why is it better? You're adding select cost to each request sent.

It is better because it does not create more intertwining between contexts as result you do not care when to close generators context. But your idea is even better - we need to drop context from the partition and close channels instead when generator cycle is ended.

Technically they have no relation to each other because those are 2 different contexts - not connected. I agree that doing some dance around closing channels could be better in principle but it is much more risky patch - as per sending to closed channel case, and it'd introduce separate closing mechanism to already existing one which would be confusing.
I think it's common pattern that dependency is created first and destructed last, which is the case here. Not ideal though as it's a weird pattern that generator has it's own waitgroup which nobody waits for.

I would rather prefer to make it straigh than further entangle logic even if it requires more effort.

That'd be best, but ending some things by context cancellation and some things by channel closing or default select I'd not say it's straight.

Agreed, but channel closing is not a way to cancel something it is a necessary precedure of closing generation cycle.
That is exactly why I would not want to propagate context cancelation as means of sending singal to stop anything.
I would want to move all that logic into stop.Flag and relay only on it.

@dkropachev
Copy link
Collaborator

Addressed at #372

@dkropachev dkropachev closed this Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants