search

njh / node-owfs

A node.js client library for the owserver protocol in owfs
https://www.npmjs.com/package/owfs
MIT License
10 stars 4 forks source link

Fix msg decode error #27

Closed colinl closed 7 years ago

colinl commented 7 years ago

Fixes issue #26

colinl commented 7 years ago

Hi Nicholas, are you able to merge this?

colinl commented 7 years ago

Just realised I meant to ask you to check that writes still work ok, I don't have the kit to check that. Thanks.

colinl commented 7 years ago

Ping?

njh commented 7 years ago

Hi Colin,

Sorry, for some reason notifications were turned off for this repository and I didn't get your messages.

Thank you very much for this fix. It looks like a good improvement. Would it be possible to add a test for this?

nick.

njh commented 7 years ago

Just realised I wrote a similar test for a STOMP server implementation - some problem - checking for a message split over two packets:

https://github.com/bbc/node-radiovis-stomp-server/blob/master/test/stomp/connection.js#L74

If you are not sure about writing tests - I can help.

Do you have an example data for what a message split over multiple packets looks like?

colinl commented 7 years ago

I will have to get back into it to remind myself what was happening. I don't think it was one message split over two packets, but a 'holding' header with no data, followed in the same packet by the real header and payload. The result was that the second header was interpreted as part of the assumed payload for the first header. I have no experience of the test harness (and not much in js).

colinl commented 7 years ago

I have confirmed that the problem occurred when a header with an empty message is immediately followed by a good message/header and the combined sequence is received by the socket in one packet. So the failing condition is a received packet starting with a header of the form: 'header': { 'version': 0, 'payload': -1, 'ret': 0, 'controlflags': 0, 'size': 0, 'offset': 0 } followed a by good message and header. Is it simple to include a test of that form?

njh commented 7 years ago

Yes, I will try and write some tests for this.

And very sorry for missing this PR for over a month.

colinl commented 7 years ago

Thanks, I will watch what you do and hopefully learn. Don't worry about the delay I was busy doing other things anyway. Will try and look at the Automatic Retry issue shortly, but it is summer so often better things to do :)