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

feat(core): handle errors in observable streams #8740

Merged

Conversation

wSedlacek
Copy link
Contributor

@wSedlacek wSedlacek commented Dec 4, 2021

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

Previously when an error was thrown in an observable as part of an SSE request the server would crash.

Issue Number: #8730

What is the new behavior?

Now errors are properly handled within SSE request so that if an error occurs then an error message event is sent and the connection is closed.

Does this PR introduce a breaking change?

  • [] Yes
  • No

Breaking changes have been removed from scope

Other information

@wSedlacek
Copy link
Contributor Author

@jmcdo29 I tried the approach of sharing logic between REST and SSE request and this is what I ended up with.
I had to adapt several functions to return Observables instead of promises so that the logic could be shared at the proxy level.

This had the side effect of sharing the behavior of unsubscribing when a request is closed from REST handlers as well.

Let me know if you like this direction and I can work on adding some test and docs.

@coveralls
Copy link

coveralls commented Dec 4, 2021

Pull Request Test Coverage Report for Build 65ceea40-0985-4f1e-b40c-0c581c2903af

  • 14 of 16 (87.5%) changed or added relevant lines in 2 files are covered.
  • 33 unchanged lines in 9 files lost coverage.
  • Overall coverage decreased (-0.1%) to 94.053%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/core/router/router-response-controller.ts 12 14 85.71%
Files with Coverage Reduction New Missed Lines %
packages/common/utils/is-uuid.ts 1 83.33%
packages/core/injector/module-token-factory.ts 1 94.29%
packages/core/router/sse-stream.ts 1 92.59%
packages/common/file-stream/streamable-file.ts 2 76.19%
packages/microservices/server/server-mqtt.ts 2 84.5%
packages/core/helpers/messages.ts 3 77.27%
packages/core/router/route-path-factory.ts 4 90.59%
packages/microservices/client/client-nats.ts 5 87.41%
packages/core/scanner.ts 14 89.45%
Totals Coverage Status
Change from base Build 2402d944-a9de-4ca1-82ac-e447b189c311: -0.1%
Covered Lines: 5662
Relevant Lines: 6020

💛 - Coveralls

@wSedlacek wSedlacek force-pushed the feat/handle-observable-stream-errors branch 2 times, most recently from fb2deb9 to d49bed1 Compare December 7, 2021 07:28
@wSedlacek wSedlacek force-pushed the feat/handle-observable-stream-errors branch from d49bed1 to fc36fae Compare December 13, 2021 05:23
@wSedlacek wSedlacek marked this pull request as ready for review December 13, 2021 05:24
@wSedlacek wSedlacek force-pushed the feat/handle-observable-stream-errors branch from fc36fae to b7f3680 Compare December 17, 2021 08:30
@kamilmysliwiec kamilmysliwiec merged commit b7378ad into nestjs:master Dec 17, 2021
@kamilmysliwiec
Copy link
Member

LGTM

}
});

return EMPTY;
Copy link
Contributor

Choose a reason for hiding this comment

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

we should definitely forward the error and let the user do this, this doesn't allow us to do so anymore.

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.

4 participants