Skip to content
This repository has been archived by the owner on May 21, 2024. It is now read-only.

Canceling stream context only in case of error #30

Closed
wants to merge 5 commits into from

Conversation

thiagodpf
Copy link
Contributor

@thiagodpf thiagodpf commented Jul 21, 2023

I imagine there's no reason to cancel the stream context unless there's actually an error.

Related to #29

grpc/stream.go Outdated Show resolved Hide resolved
@olegbespalov
Copy link
Collaborator

Hey @thiagodpf !

Thanks again for the contribution and for actually pointing out the issue. I have a question, does this fix solve your issue?

I'm asking because from the top of my head, I still see the issue with the bi-directional streams since we still do close(s.done) that they will stop receiving messages 😢

@thiagodpf
Copy link
Contributor Author

Hi @olegbespalov !

You're right, I think I biased my analysis because I was debugging k6 and my application at the same time without taking into account the independence between the two programs, I really owed a test scenario in this PR.

At that moment, my intention was to stop the premature cancellation of the context and I ended up not realizing that the structure that encapsulates the stream was also being closed. I think it's better to cancel this PR so as not to pollute your issues board. 😅

Unfortunately I lack knowledge of the code to be able to propose a decent solution, my fault. 😁

@thiagodpf thiagodpf closed this Aug 8, 2023
@olegbespalov
Copy link
Collaborator

No worries at all! I'll try to prioritize that and suggest a fix in an upcoming cycle. Anyway, thanks for your input!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants