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 context propagation and work cancellation #20

Merged
merged 6 commits into from
Mar 15, 2023

Conversation

nassah221
Copy link
Collaborator

@nassah221 nassah221 commented Mar 15, 2023

Addresses #16. It also contains the changes from #19 as it builds up on it. Please let me know if I should stack this on top of #19 rather than base it to main or convert #19 to draft.

While the current method does work, the binary is killed with os.Exit() in main.go. With appropriate propagation of context to derived goroutines, the code is more readable and concise.

rtmp and thumnailer code seem to have the bulk of he changes. Please keep in mind that It is a possibilty that we might have to revisit this again in the future because some of the inputs don't make use of context at all currently, we will have to discover cause and origin of cancellation for each of them and then handle it gracefully.

@clone1018
Copy link
Member

Addresses #16. It also contains the changes from #19 as it builds up on it. Please let me know if I should stack this on top of #19 rather than base it to main or convert #19 to draft.

Good like it is! Whatever process makes me not block your forward progress.

Copy link
Member

@clone1018 clone1018 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested this locally, played around with a good number of exit conditions and they did seem to work well!

I'll merge this now and get it out on a production server tomorrow for monitoring!

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