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

Stop useless panicking in context and render #2150

Merged
merged 16 commits into from
Feb 12, 2023

Conversation

eeonevision
Copy link
Contributor

@eeonevision eeonevision commented Nov 25, 2019

When user resets connection to the server, the go's http returns "broken pipe -> write tcp...".
There is no needed to panicking because it's common case.

Instead of this I suggest to catch any errors, excepting broken pipe (syscall.EPIPE error), in server-side initialised logger.

This will improve overall performance of gin and will very helpful to log any other render errors to log.

@codecov
Copy link

codecov bot commented Nov 25, 2019

Codecov Report

Merging #2150 (f32da2d) into master (3010cbd) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2150   +/-   ##
=======================================
  Coverage   98.63%   98.63%           
=======================================
  Files          42       42           
  Lines        3140     3141    +1     
=======================================
+ Hits         3097     3098    +1     
  Misses         29       29           
  Partials       14       14           
Flag Coverage Δ
98.63% <100.00%> (+<0.01%) ⬆️
go-1.16 ∅ <ø> (?)
go-1.17 98.53% <100.00%> (+<0.01%) ⬆️
go-1.18 98.53% <100.00%> (+<0.01%) ⬆️
go-1.19 98.63% <100.00%> (+<0.01%) ⬆️
macos-latest 98.63% <100.00%> (+0.09%) ⬆️
ubuntu-latest 98.63% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
context.go 97.97% <100.00%> (+0.01%) ⬆️
render/json.go 82.75% <100.00%> (-0.39%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

vkd
vkd previously approved these changes Nov 27, 2019
@LiFeAiR
Copy link

LiFeAiR commented Nov 29, 2019

This PR close two issues:
#1827
#1770

@thinkerou thinkerou added this to the 1.6 milestone Feb 25, 2020
@thinkerou thinkerou modified the milestones: 1.6, 1.7 Feb 28, 2020
@thinkerou thinkerou modified the milestones: 1.7, v1.8 Oct 27, 2020
@daheige
Copy link
Contributor

daheige commented Apr 29, 2021

When user resets connection to the server, the go's http returns "broken pipe -> write tcp...".
There is no needed to panicking because it's common case.

Instead of this I suggest to catch any errors, excepting broken pipe (syscall.EPIPE error), in server-side initialised logger.

This will improve overall performance of gin and will very helpful to log any other render errors to This processing method is very good. In 2019, I found that there was a problem with the panic processing here, which often led to the online service CPU full, abnormal service, and online failure. I also mentioned issue to gin in 2019, but it hasn't been solved. Now from your submission, it's really a great way to deal with it. I support this mode. Thank you very much

When user resets connection to the server, the go's http returns "broken pipe -> write tcp...".
There is no needed to panicking because it's common case.

Instead of this I suggest to catch any errors, excepting broken pipe (syscall.EPIPE error), in server-side initialised logger.

This will improve overall performance of gin and will very helpful to log any other render errors to log.

This processing method is very good. In 2019, I found that there was a problem with the panic processing here, which often led to the online service CPU full, abnormal service, and online failure. I also mentioned issue to gin in 2019, but it hasn't been solved. Now from your submission, it's really a great way to deal with it. I support this mode. Thank you very much!

daheige
daheige previously approved these changes May 1, 2021
@Tevic
Copy link
Contributor

Tevic commented Jan 26, 2022

Is there any update of this PR?

@mayankvaid88
Copy link

mayankvaid88 commented Jan 10, 2023

Can we merge this PR ?
Panic is redundant. Error can be handled gracefully.

@danmux
Copy link

danmux commented Jan 10, 2023

@mayankvaid88 - I hope you are asking can we merge the PR? :)

@mayankvaid88
Copy link

@mayankvaid88 - I hope you are asking can we merge the PR? :)

Yeah. My bad :)

@eeonevision
Copy link
Contributor Author

eeonevision commented Jan 10, 2023

Hello! Thanks to all for paying attention to this PR.
Seems like our maintainers just forgot about this issue in many others.

@appleboy, @thinkerou could you merge this into the main branch, please?
If not, it would be nice to provide some feedback to us.

UPD: Fixed unit tests and merged upstream branch into forked.

danmux
danmux previously approved these changes Jan 10, 2023
@eeonevision eeonevision dismissed stale reviews from danmux, ghost , and daheige via d0b7ad2 January 10, 2023 12:50
@krupyansky
Copy link

@appleboy @thinkerou approve the merge request please.

@thinkerou
Copy link
Member

@appleboy please review the pull request, thanks!

1 similar comment
@krupyansky
Copy link

@appleboy please review the pull request, thanks!

@MatheusLFreitas
Copy link

MatheusLFreitas commented Feb 1, 2023

People, do we really need this approval from @appleboy?
Isn't there other contributors that could approve this for us?
This is too important and there is several related issues and we are blocked because we are still waiting for a person that is not responding for over 4 years now.
Couldn't this review be assigned to somebody else?

For example @danmux or @vkd or @daheige

@danmux
Copy link

danmux commented Feb 1, 2023

For example @danmux or @vkd or @daheige

It has to be one of the maintainers before it can be merged

@appleboy appleboy merged commit 0c96a20 into gin-gonic:master Feb 12, 2023
@appleboy
Copy link
Member

Thanks all guys. We will release v1.9 version asap.

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

Successfully merging this pull request may close these issues.