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

Figure out the right way to cancel in progress work #16

Open
clone1018 opened this issue Mar 7, 2023 · 2 comments
Open

Figure out the right way to cancel in progress work #16

clone1018 opened this issue Mar 7, 2023 · 2 comments
Labels
architecture enhancement New feature or request good first issue Good for newcomers

Comments

@clone1018
Copy link
Member

A live stream can stop (for whatever reason) either from the Protocol / Input layer, or from the Control layer. This bi-directional cancellation is currently a mess, and there's some hacky code in place to make it work.

Reasons for Cancellation

  • Control
    • Service / Orchestrator failure (unexpected or expected)
    • Control shutdown
  • Protocol / Input
    • Specific disconnect request
    • Network io error

Options

It seems like context may be appropriate for this, however I don't have a ton of experience using it.

Alternatively, depending on the Control architecture, we could use callbacks -- though that's not very "go" like.

@clone1018 clone1018 added enhancement New feature or request good first issue Good for newcomers architecture labels Mar 7, 2023
@nassah221
Copy link
Collaborator

I have been reviewing the code around the current implementation. While it works, I think doing bi-directional cancellation is better done through the use of channels as well as context.

Generally, context is meant to be passed from a parent to derived tasks or subtasks. The parent is then responsible for cancelling any subtasks it may have created. Storing context in a struct is also frowned upon.

For clarity, here's what I have in mind.

  1. When the cancellation is directed from the control to the session stream, there should be a parent context for OS signals which is propagated from main.go to the code responsible for handling new streams. This context is cancelled when server is killed/restarted.

  2. When the cancellation is directed from the session stream to control due to bad io etc. the use of channels would be more appropriate. AFAIK, the stream type is responsible for cleanup/removal itself. Instead of propagating context from a child/spawned task to a parent task, we could use a oneshot channel to signal cleanup/cancellation to control.

WDYT @clone1018

@clone1018
Copy link
Member Author

I like the idea of using a simpler setup with channels and contexts. I implemented an experimental version of this in 3004b21 however they are stored in a struct. It didn't seem clear to me how to keep access to them over time and handle cancellation inside the inputs. Looking forward to seeing your improved implementation!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
architecture enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants