Skip to content

Comments

Improvements to middlewares#1671

Merged
pcapriotti merged 5 commits intodevelopfrom
pcapriotti/middleware-improvements
Jul 22, 2021
Merged

Improvements to middlewares#1671
pcapriotti merged 5 commits intodevelopfrom
pcapriotti/middleware-improvements

Conversation

@pcapriotti
Copy link
Contributor

@pcapriotti pcapriotti commented Jul 16, 2021 •
edited
Loading

Summary of changes

  • Reorder middleware application in all services: catchError has to run before gzip to get a chance to see the response body uncompressed.
  • Simplify lazyResponseBody: no need to use STM there.
  • Avoid unnecessary roundtrip from ByteString to Text when wrapping errors.

This (the first change, specifically) should fix the strange error response in some integration tests (like in #1662).

Checklist

  • The PR Title explains the impact of the change.
  • The PR description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.
  • If end-points have been added or changed: the endpoint / config-flag checklist (see Wire-employee only backend wiki page) has been followed.
  • If a schema migration has been added, I ran make git-add-cassandra-schema to update the cassandra schema documentation.
  • Section Unreleased of CHANGELOG.md contains the following bits of information:
    • A line with the title and number of the PR in one or more suitable sub-sections.
    • If /a: measures to be taken by instance operators.
    • If /a: list of cassandra migrations.
    • If public end-points have been changed or added: does nginz need upgrade?
    • If internal end-points have been added or changed: which services have to be deployed in a specific order?

@pcapriotti
Run error catching middleware before gzip
e5c2e5d 
The `catchError` middleware was running over gzipped responses, and this
caused the wrapping logic to fail, and to display a gzip-encoded error
message.
@pcapriotti
Avoid unnecessary conversion to Text
bf8270d 
This prevents the response body to being preemptively converted to Text
when wrapping an error response inside a JSON object.
@pcapriotti
Use IORef instead of TVar for extracting body
388e658 
There is no need to use STM in `lazyResponseBody`, since it is not
interacting with any other STM action, and just using TVars inside
`atomically` blocks.
pcapriotti added a commit that referenced this pull request Jul 16, 2021
@pcapriotti
Merge middleware-improvements branch (#1671)
0d71244 
This should fix the error reporting and show what the actual problem is.
mdimjasevic
mdimjasevic approved these changes Jul 16, 2021
View reviewed changes
Copy link
Contributor

@mdimjasevic mdimjasevic left a comment

Choose a reason for hiding this comment

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

It was a nice discussion why this error was happening!

@pcapriotti pcapriotti merged commit f9729a4 into develop Jul 22, 2021
@pcapriotti pcapriotti deleted the pcapriotti/middleware-improvements branch July 22, 2021 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fisx fisx fisx approved these changes

+1 more reviewer

@mdimjasevic mdimjasevic mdimjasevic approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@pcapriotti @mdimjasevic @fisx