Skip to content

fix(WebSocketSubject): Closes socket when closing socket observer #7077

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bever1337
Copy link

Description:

When the output observer is closed because of de-serialization errors, ensure the WebSocketSubject is reset and the WebSocket is closed.

Related issue (if exists): (#5312)

@bever1337
Copy link
Author

bever1337 commented Sep 30, 2022

I don't believe there's any marble testing to be done since this work is making native features observable. In my experience, there is no equivalent framework like msw for mocking and testing websockets. This is my first contribution to RxJS, and I would appreciate anyone's time ensuring my above statement is accurate. Additionally, are there other changes that need to be made in RxJS to support this? Other tests? Docs? Thank you

@bever1337 bever1337 changed the title fix(WebSocketSubject): Closes socket when closing socket subject obse… fix(WebSocketSubject): Closes socket when closing socket observer Sep 30, 2022
@bever1337
Copy link
Author

bever1337 commented Sep 30, 2022

I need to research further, but I think the multiplexer can also create the same state where the websocket subject has errored but the underlying socket remains open. I don't want to conflate issues and the multiplexer code is trickier to change because it has more error paths.

@maknapp
Copy link
Contributor

maknapp commented Oct 10, 2022

There is a MockWebSocket in webSocket-spec.ts to write tests with. There seems to be only one test for deserializer errors, and it does not check for ws.close. There are certainly more tests to add in this area.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants