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

Add RST packet processing fixes #156

Merged
merged 3 commits into from
Dec 8, 2017
Merged

Conversation

GiedriusM
Copy link
Collaborator

This PR fixes several issues when processing RST packets, including original issue #154:

  • Server sends RST packet with token
  • Server sends NON '5.00' "Empty message must be empty" after the RST
  • Server sends ACK after the RST
  • In rare occasions client may send a RST message in response to RST
  • Request does not finish after receiving RST (cleanUp is not called, so the program just "hangs" without finishing)

Could anyone review the changes? @mcollina @neophob?

Also, I was unsure how should I handle a RST as a response - at first I did a separate req.on('reset') event, but then just slapped RST into response as it does not alter the API. Which do you guys think is better?

@neophob
Copy link
Collaborator

neophob commented Nov 19, 2017

Hi @GiedriusM - looks nice. I will test this next week to see if there any side effects.

@GiedriusM
Copy link
Collaborator Author

Thanks, I'll add you as a reviewer then.

@GiedriusM
Copy link
Collaborator Author

Merged with the (hopefully) working fix. Tests seem to pass without any disturbances.

@neophob
Copy link
Collaborator

neophob commented Dec 7, 2017

finally - could test with my app, LGTM!

@GiedriusM
Copy link
Collaborator Author

Thanks, @neophob! I'll merge it once you validate the review.

Copy link
Collaborator

@neophob neophob left a comment

Choose a reason for hiding this comment

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

LGTM

@GiedriusM GiedriusM merged commit 9335cbc into coapjs:master Dec 8, 2017
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