[pr] bugfix/bogus-quit-detected #3
Conversation
…d, if that character was received as a single packet in the middle of reading data for another command.
![pullrequest[bot]](https://avatars.githubusercontent.com/in/3434?s=60&v=4){kind=small}
There was a problem hiding this comment.
Left a couple of clarifying questions inline just so I can understand exactly what the fix is here. Nice job adding some test coverage that would have caught this.
Was this helpful? Let us know!
test/protocol.js
Outdated
| it(`should store ${test.ext} data with a (${test.cmd}) command (client write packet size = ${test.packetSize})`, () => { | ||
| // Insert 'q' character ('Quit' command) into the GUID, to catch subtle protocol errors when packet size is 1 | ||
| if(test.packetSize === 1) { | ||
| test.data.guid[test.data.guid.length - 1] = 113; |
There was a problem hiding this comment.
not a big deal since you have a comment in place, but could possible make this more readable to use = 'q'.charCodeAt(0); instead of the evaluated value of 113.
|
|
||
| // Quit? | ||
| if (data[data.length - 1] === CMD_QUIT) { | ||
| if (!readState.didParseCmd && data[data.length - 1] === CMD_QUIT) { |
There was a problem hiding this comment.
so just to clarify, the idea behind using the !readState.disParseCmd check first is that the q would always come at the start of the command, but not in subsequent characters? is it possible that this could still fail if the first character of the guid is a q?
There was a problem hiding this comment.
The real problem here is that the protocol should have made the QUIT command a 2 char length as well. So I think this is the best I can do - if a command (of a 2 char variety) is already parsed, then the QUIT command is effectively disabled until the state is reset. I think the only questionable assertion in this is if a 2 char command is not yet parsed, 'q' all by itself will signal a quit. The protocol tests seem really stable now, at any rate.
There might be an overall better way to architect this low level protocol parser to better encapsulate all of that logic .. I am loathe to re-write it at this point though :) suggestions welcome.
There was a problem hiding this comment.
Gotcha, yeah I can't really think of an easy solution given your point around needing 2 characters of data. I think your solution here is good and should help fix or at least make it much more rare to occur. Thanks for the info!
test/protocol.js
Outdated
| it(`should store ${test.ext} data with a (${test.cmd}) command (client write packet size = ${test.packetSize})`, () => { | ||
| // Insert 'q' character ('Quit' command) into the GUID, to catch subtle protocol errors when packet size is 1 | ||
| if(test.packetSize === 1) { | ||
| test.data.guid[test.data.guid.length - 1] = 'q'.charCodeAt(0); |
There was a problem hiding this comment.
I am slightly curious regarding the comments above if you were to make this test.data.guid[0] = 'q'.charCodeAt(0); would the tests with packet-size of 1 fail?
There was a problem hiding this comment.
I wouldn't expect it to, but just in case I'll add it to the test. It shouldn't matter positionally where the q is encountered .. as long as it is somewhere after a 2 char command is received, it would have triggered the bug.
Fixed bug where a ‘q’ character is mis-interpreted as a quit command, if that character was received as a single packet in the middle of reading data for another command.