-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Ensure to close StreamFrame channel #12248
Conversation
Should I do anything else to make the CI successful? Please tell me if there is, thank you. |
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.
Hi @nakabonne! Sorry for the delay in reviewing this. I'm looking at the only call site we currently have for this function in Nomad (ref api/fs.go#L341-L357
) and it looks like the results end up being the same either way in our current implementation. But this change probably makes it more clear for callers, so LGTM.
I'm not worried about the CI we see failing here, as those are for docs and need approval to run anyways. But for some reason none of the unit tests ran for this PR and I'm not sure why. I'm going to try to rebase this PR on main
and see if I can get the build to kick off.
Tests are green, so LGTM! Thanks @nakabonne! |
@tgross Thank you for maintaining this great project! |
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
A tiny fix to close the StreamFream channel before finishing the streaming by the
<-cancel
channel.Let's say you use FrameReader.Read(), now we have no way to quit the select block even when the streaming finished. (Actually we can pass
cancelCh chan struct{}
to NewFrameReader but I'd like to stop reading when<-ctx.Done()
)Therefore, I would say it should close immediately as soon as the streaming finished.