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

logging: library code should not crash #1862

Closed
ghasemloo opened this issue Mar 18, 2020 · 4 comments · Fixed by #3076
Closed

logging: library code should not crash #1862

ghasemloo opened this issue Mar 18, 2020 · 4 comments · Fixed by #3076

Comments

@ghasemloo
Copy link

@ghasemloo ghasemloo commented Mar 18, 2020

Client

Stackdriver Logging

Environment

all

Code

replace panics

0d861fa

Expected behavior

library code should not crash (except at startup)

Actual behavior

library code crashes when request is nil

@ghasemloo ghasemloo changed the title logging: library code should not clash logging: library code should not crash Mar 18, 2020
@codyoss
Copy link
Member

@codyoss codyoss commented Mar 20, 2020

From looking at both of these they seem like actual valid reasons to panic. I could be missing something though. Did you trigger these panics while your code was running?

@codyoss
Copy link
Member

@codyoss codyoss commented Apr 3, 2020

Closing due to lack of response. Please reopen if you have more information to share.

@codyoss codyoss closed this Apr 3, 2020
@ghasemloo
Copy link
Author

@ghasemloo ghasemloo commented Apr 13, 2020

Yes, it did, the request was nil I think.

Based on Google's internal go style library code should not panic: go/gocomments#dont-panic
Filed here instead of internally since this seems to be the source of truth for the code.

@ghasemloo
Copy link
Author

@ghasemloo ghasemloo commented Apr 13, 2020

@jba fyi

@codyoss codyoss reopened this Apr 14, 2020
@codyoss codyoss self-assigned this May 12, 2020
@codyoss codyoss assigned simonz130 and unassigned codyoss Sep 29, 2020
@0xSage 0xSage assigned 0xSage and unassigned simonz130 Oct 9, 2020
gcf-merge-on-green bot pushed a commit that referenced this issue Oct 29, 2020
fixes: #1862 

Changes: 
- No longer panic on User introduced errors. Instead we log the error and let program proceed
- We log.Fatalf("ptypes.TimestampProto(time.Unix(0,0)) failed: %v", err) to exit program, since it's a fatal error/likely not induced by Users
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants