Skip to content

Handle RPC errors as well as message-inline errors#6589

Merged
stuhood merged 3 commits into
pantsbuild:masterfrom
twitter:dwagnerhall/remoting/status
Oct 5, 2018
Merged

Handle RPC errors as well as message-inline errors#6589
stuhood merged 3 commits into
pantsbuild:masterfrom
twitter:dwagnerhall/remoting/status

Conversation

@illicitonion

Copy link
Copy Markdown
Contributor

This required a minor change to grpcio
(pantsbuild/grpc-rs@4dfafe9)
to not throw away the information from the grpc stack.

This required a minor change to grpcio
(pantsbuild/grpc-rs@4dfafe9)
to not throw away the information from the grpc stack.
Doesn't work right now because servers can't fail with a Status
.into_future()
// If there was a response, drop the _stream to disconnect so that the server doesn't keep
// the connection alive and continue sending on it.
.map(|(maybe_operation, _stream)| maybe_operation)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Worth explicitly dropping with mem::drop and placing the comment there? On the other hand, maybe doing it this way is less confusing for a new rust user because it doesn't make mem::drop seem magical.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

}
}

//#[test] // TODO: Unignore this test when the server can actually fail with status protos.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Worth creating a ticket for this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

@stuhood stuhood merged commit b1e73b5 into pantsbuild:master Oct 5, 2018
@stuhood stuhood deleted the dwagnerhall/remoting/status branch October 5, 2018 16:08
stuhood added a commit to twitter/pants that referenced this pull request Oct 6, 2018
stuhood pushed a commit that referenced this pull request Oct 6, 2018
Fix a merge conflict between #6589 and #6581.
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