Skip to content

Stop useless panicking in context and render - #2150

Merged
appleboy merged 16 commits into
gin-gonic:masterfrom
eeonevision:master
Feb 12, 2023
Merged

appleboy merged 16 commits into
gin-gonic:masterfrom
eeonevision:master

Conversation

@eeonevision

@eeonevision eeonevision commented Nov 25, 2019 •

Copy link
Copy Markdown
Contributor

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

codecov Bot commented Nov 25, 2019 •

Copy link
Copy Markdown

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

LiFeAiR commented Nov 29, 2019

Copy link
Copy Markdown

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

daheige commented Apr 29, 2021

Copy link
Copy Markdown
Contributor

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

Tevic commented Jan 26, 2022

Copy link
Copy Markdown
Contributor

Is there any update of this PR?

@danmux

danmux commented Dec 6, 2022 •

Copy link
Copy Markdown

This will also close #3351 and #2676

@mayankvaid88

mayankvaid88 commented Jan 10, 2023 •

Copy link
Copy Markdown

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

@danmux

danmux commented Jan 10, 2023

Copy link
Copy Markdown

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

@mayankvaid88

Copy link
Copy Markdown

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

Yeah. My bad :)

@eeonevision

eeonevision commented Jan 10, 2023 •

Copy link
Copy Markdown
Contributor Author

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
@ghost

ghost commented Jan 16, 2023

Copy link
Copy Markdown

@appleboy @thinkerou approve the merge request please.

@thinkerou

Copy link
Copy Markdown
Member

@appleboy please review the pull request, thanks!

1 similar comment
@ghost

ghost commented Jan 24, 2023

Copy link
Copy Markdown

@appleboy please review the pull request, thanks!

@MatheusLFreitas

MatheusLFreitas commented Feb 1, 2023 •

Copy link
Copy Markdown

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

danmux commented Feb 1, 2023

Copy link
Copy Markdown

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
Copy Markdown
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.

10 participants