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

fix(gateway): do not act on template errors #515

Merged
merged 3 commits into from
Nov 22, 2023
Merged

fix(gateway): do not act on template errors #515

merged 3 commits into from
Nov 22, 2023

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Nov 21, 2023

Fixes #487. When we try to execute a template, we have already written the header. In some places, we were already ignoring the template execution error. In others, we were calling webError. That would lead to a duplicate header writing warning.

There is not much that can be done if a template fails to execute: we could perhaps log it, but so far what we've done in other places is to ignore. A way to circumvent this would be to write to a buffer and then write to http.ResponseWriter. That would allow us to catch the template error, but writing can always fail (e.g., broken connection).

I wouldn't care much about this and I think just ignoring the template writing error is probably fine. If errors happen, they will most likely be writing errors, so broken connections and other network issues, most likely.

@hacdias hacdias self-assigned this Nov 21, 2023
@hacdias hacdias requested a review from lidel as a code owner November 21, 2023 12:18
@hacdias hacdias requested a review from a team as a code owner November 21, 2023 12:20
Copy link

codecov bot commented Nov 21, 2023

Codecov Report

Merging #515 (5bfcf4f) into main (fe55533) will decrease coverage by 0.07%.
The diff coverage is 35.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #515      +/-   ##
==========================================
- Coverage   65.58%   65.52%   -0.07%     
==========================================
  Files         207      207              
  Lines       25540    25543       +3     
==========================================
- Hits        16750    16736      -14     
- Misses       7323     7335      +12     
- Partials     1467     1472       +5     
Files Coverage Δ
gateway/handler_unixfs_dir.go 67.27% <100.00%> (ø)
gateway/handler_codec.go 62.85% <60.00%> (+0.77%) ⬆️
gateway/errors.go 83.33% <25.00%> (-2.14%) ⬇️
gateway/handler.go 76.54% <0.00%> (-0.14%) ⬇️

... and 7 files with indirect coverage changes

gateway/handler.go Show resolved Hide resolved
@hacdias hacdias requested a review from lidel November 22, 2023 14:58
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Thanks! Lgtm.

@lidel lidel merged commit d06f7ff into main Nov 22, 2023
14 checks passed
@lidel lidel deleted the fix-487 branch November 22, 2023 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

http: superfluous response.WriteHeader
2 participants