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

Refactor ping packet handling in WebsocketTransport #507

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

Conversation

URVL
Copy link

@URVL URVL commented Dec 5, 2024

Server-server connection ping error
Снимок экрана 2024-12-04 в 16 06 48

  • Changed PING_PACKET from a Buffer to an object for improved clarity.
  • Updated the method call from this.write(PING_PACKET) to this.send(PING_PACKET) to reflect the new structure.

This change enhances the readability and maintainability of the code related to pinging in the WebSocket transport layer.

  • [ +] tests and linter show no problems (npm t)
  • [+ ] tests are added/updated for bug fixes and new features
  • [+ ] code is properly formatted (npm run fix)
  • [+ ] description of changes is added in CHANGELOG.md
  • [ +] update .d.ts typings

- Changed PING_PACKET from a Buffer to an object for improved clarity.
- Updated the method call from `this.write(PING_PACKET)` to `this.send(PING_PACKET)` to reflect the new structure.

This change enhances the readability and maintainability of the code related to pinging in the WebSocket transport layer.
@nechaido
Copy link
Member

nechaido commented Dec 5, 2024

The issue is that when ping is send as a buffer - it is treated as a binary here
Here:
https://github.com/metarhia/metacom/blob/master/lib/server.js#L207

Which results in
https://github.com/metarhia/metacom/blob/master/lib/streams.js#L18

Trring to read 32 bytes from a 16byte buffer, this lead to server failure.

Alternative solution would be to use "{}" without the buffer and test to see if it works that way.

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