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.

Sign up for GitHub

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

Jump to bottom

"WebSocketSubject.error must be called with an object with an error code" when serializer throws exception #7111

Open
voliva opened this issue Nov 9, 2022 · 3 comments · May be fixed by #7114
Open

"WebSocketSubject.error must be called with an object with an error code" when serializer throws exception #7111

voliva opened this issue Nov 9, 2022 · 3 comments · May be fixed by #7114
Labels
bug Confirmed bug

Comments

@voliva
Copy link
Contributor

voliva commented Nov 9, 2022 •
edited

Describe the bug

When sending a message to a WebSocketSubject that causes the serializer to throw an exception, rxjs throws instead a different exception:

TypeError: WebSocketSubject.error must be called with an object with an error code, and an optional reason: { code: number, reason: string }
    at Object.eval [as error] (WebSocketSubject.js:156:26)
    at ConsumerObserver.error (Subscriber.js:118:25)
    at Subscriber._error (Subscriber.js:82:24)
    at Subscriber.error (Subscriber.js:59:12)
    at Object.eval [as next] (WebSocketSubject.js:145:31)
    at ConsumerObserver.next (Subscriber.js:108:25)
    at Subscriber._next (Subscriber.js:78:22)
    at Subscriber.next (Subscriber.js:51:12)
    at ReplaySubject._subscribe (ReplaySubject.js:55:18)
    at Observable._trySubscribe (Observable.js:43:19)
    at Subject._trySubscribe (Subject.js:117:43)
    at eval (Observable.js:37:115)
    at errorContext (errorContext.js:28:5)
    at Observable.subscribe (Observable.js:35:47)
    at socket.onopen [as _onopen] (WebSocketSubject.js:168:32)

Expected behavior

The original error should be thrown instead

Reproduction code

const socket$ = webSocket({
  url: 'http://www.example.com/',
  serializer: message => {
    return JSON.stringify(message);
  }
});

socket$.subscribe({
  next: (v) => console.log('next', v),
  error: e => console.error('caught', e),
  complete: () => console.log('complete')
});

socket$.next({
  value: BigInt(5)
});

Reproduction URL

https://codesandbox.io/s/lively-river-dqm1qr?file=/src/index.ts:197-527

Version

7.5.7

Environment

No response

Additional context

This often creates hard-to-debug errors, as you can't know what was the original exception that happened in the serializer.

(The example code is trivial, but it's tough when the serialiser works with binary streams).
@voliva voliva linked a pull request Nov 11, 2022 that will close this issue
Open
@benlesh benlesh added the bug Confirmed bug label Dec 3, 2022
@benlesh
Copy link
Member

benlesh commented Dec 3, 2022

I see the issue. But it looks like the solution in the related PR isn't quite right. We don't want anything to synchronously throw... Rather the WebSocketSubject should emit an error, while sending an error code to the remote socket. 🤔 It requires some thought.

@benlesh benlesh added the AGENDA ITEM Flagged for discussion at core team meetings label Dec 3, 2022
@benlesh benlesh removed the AGENDA ITEM Flagged for discussion at core team meetings label Dec 14, 2022
@benlesh
Copy link
Member

benlesh commented Dec 14, 2022

Okay... thinking about this, what it should do when a serialization error occurs is:

  1. Notify the user of the error in the error handler
  2. Close the Web Socket
  3. Close the Web Socket Subject

The web socket subject should be "retryable" at this point however.

Note that it should NOT synchronously throw an error when calling myWebSocketSubject.next(someNonSerializableValue)

@voliva
Copy link
Contributor Author

voliva commented Dec 15, 2022

Thank you for looking into this @benlesh. I'll edit my PR with this sometime this week if it hasn't been done already

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Confirmed bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(webSocket): manage serializer throwing exceptions voliva/rxjs
2 participants
@benlesh @voliva