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

drpcmanager: cancel stream with error when terminated #31

Closed
wants to merge 1 commit into from

Conversation

kylecarbs
Copy link
Contributor

Terminations were canceling streams with context.Canceled instead
of surfacing the error, causing unexpected behavior.

Terminations were canceling streams with `context.Canceled` instead
of surfacing the error, causing unexpected behavior.
@kylecarbs
Copy link
Contributor Author

@zeebo could you take a look?

@zeebo
Copy link
Collaborator

zeebo commented Jun 15, 2022

Hmm.. I think this changes some semantics. Specifically, a MsgRecv returning an io.EOF indicates that the other end has cleanly closed the stream. That is now happening even in the case of a "hard close" of the underlying transport. That's why the manager was ignoring the error and terminating the stream with context.Canceled. The test as written was to enforce that property, actually.

@kylecarbs
Copy link
Contributor Author

Is there another way we should surface this error?

Because this error wasn't being surfaced, it was very difficult for us to log the true reason a stream was being terminated.

@zeebo
Copy link
Collaborator

zeebo commented Jun 15, 2022

Perhaps the manager code can check to see if the error was io.EOF and terminate with context.Canceled, and otherwise terminate with the provided error. I think that would be fine to both surface unexpected transport errors as well as maintain the property that clean, intentional closes can be detected.

@zeebo
Copy link
Collaborator

zeebo commented Jun 15, 2022

Or, maybe we do something like io.ReadFull and change io.EOF into io.ErrUnexpectedEOF instead of the previously suggested context.Canceled. I think that's better.

mafredri added a commit to coder/coder that referenced this pull request Jul 12, 2022
The storj/drpc#31 PR was not merged, but was
replaced by:

storj/drpc@9206537

This should fix the underlying issue fixed in the fork.
mafredri added a commit to coder/coder that referenced this pull request Jul 12, 2022
The storj/drpc#31 PR was not merged, but was
replaced by:

storj/drpc@9206537

This fixes the underlying issue fixed in the fork.
@storjrobot
Copy link

This pull request has been mentioned on Storj Community Forum (official). There might be relevant details there:

https://forum.storj.io/t/introducing-drpc-our-replacement-for-grpc/13486/13

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.

None yet

3 participants