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

unmergeable types should panic instead of logging #160

Closed
aybabtme opened this issue Mar 29, 2016 · 5 comments
Closed

unmergeable types should panic instead of logging #160

aybabtme opened this issue Mar 29, 2016 · 5 comments

Comments

@aybabtme
Copy link

In https://github.com/golang/protobuf/blob/master/proto/clone.go#L204, logging the problem instead of returning-an-error/panic'ing leads to silent failures, loss of data and subtle rage inducing bugs.

Since the API would be broken if the call signature was modified to return an error, I suggest that the call panic instead of logging. This way code that incorrectly expects proto.Merge to merge their types would panic (and hopefully be fixed) instead of proceeding silently with corrupted state.

@dsymonds
Copy link
Contributor

Maybe, but it shouldn't be possible to even trigger that line. It was mainly there for development.

Do you have a situation where it has actually arisen?

@aybabtme
Copy link
Author

Yes, I'm seeing it occur quite often. I'm away for the next few weeks but
after that, I'd be happy to try and make a reproducer.

On Wednesday, 13 April 2016, David Symonds [email protected] wrote:

Maybe, but it shouldn't be possible to even trigger that line. It was
mainly there for development.

Do you have a situation where it has actually arisen?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#160 (comment)

Cheers,

Antoine Grondin

Live long and prosper

@dsymonds
Copy link
Contributor

Please do. I'm not aware of any way to trigger that log line.

@aybabtme
Copy link
Author

Should be safe to panic then ;)

On Wednesday, 13 April 2016, David Symonds [email protected] wrote:

Please do. I'm not aware of any way to trigger that log line.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#160 (comment)

Cheers,

Antoine Grondin

Live long and prosper

@dsnet
Copy link
Member

dsnet commented Feb 14, 2018

The dev branch uses a new implementation of Merge. Over time the old will be killed off.
Track #479 for when the merge of dev to master occurs.

@dsnet dsnet closed this as completed Feb 14, 2018
@golang golang locked and limited conversation to collaborators Jun 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants