Fix device log bug #2
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When we get the device log, the C++ code starts to print on the stdout and in the JavaScript code we listen for the data event. If there is enaugh time between each message which is printed on the stdout the data event is raised with only one message. If the C++ code starts to print on the stdout too fast (which is the case with the device log), NodeJS buffers all the data and emits data event with all the buffered data. This breaks out code because we expect only one message. The solution for this problem is to write method which will "parse" the buffered data. This method should check the header of the received buffered data and read message from the buffered data. If there is full message in the buffered data we will read it. If there is no full message we should store the unfinished message and on the next received buffered data we should concat the unfinished message and the new data and try to parse the concatenated message. We rely on that the first message should have header with length. After that the method will make sure that the unfinished message will always begin with header with length and that each recursive call will receive data which begins with header with length.
While testing the new method with 7 threads which print on the stdout we noticed that at some point the data which is printed on the stdout gets malformed. For example with this message:
`{"message1":"Feb 10 13:59: 28 dragon - iPhone6 timed[65] <Notice> : (Error)TimeLink : B76E090A - 9807 - 4AA3 - BB01 - 3CDE3DD48D55(4 \n- TMLSSourceDevice) send failure(Error Domain = com.apple.identityservices.error Code = 23 UserInfo=0x12fe326f0 {NSLocalizedDescription=Timed out, NSUnderlyingError=0x12fe34190 The operation couldnt be completed. (com.apple.ids.idssenderrordomain error 12.)}), will retry"}`
we receive random number of messages before we receive one message with additional `}`. With one thread we do not receive malformed output. To solve this problem we need to add mutex in the print method and make it thread safe.
TsvetanMilanov
force-pushed
the
milanov/fix-device-log-bug
branch
from
1daf259 to
9a9c464
Compare
| void print(const char* str) | ||
| { | ||
| // We need to lock the print method because in some cases we print different parts of messages | ||
| // from different threads. | ||
| print_mutex.lock(); |
There was a problem hiding this comment.
If we decide to lock printing after all can we place the mutex on the device log operations only so as to not slow down all the other operations 🤔
There was a problem hiding this comment.
I prefer using the mutex whenever we print on the terminal as this way we'll guarantee correct order of messages even if only part of them is sent to stdout's on data event
rosen-vladimirov
suggested changes
Feb 14, 2017
ios-device-lib-stdio-handler.js
Outdated
| // Less than one message in the buffer. | ||
| // Store the unfinished message untill the next call of the function. | ||
| this._unfinishedMessage = data; | ||
| } else if (data.length - 4 > messageLength) { |
There was a problem hiding this comment.
why - 4, shouldn't this be - HeaderSize?
ios-device-lib-stdio-handler.js
Outdated
| const concatenatedMessage = Buffer.concat([this._unfinishedMessage, data]); | ||
|
|
||
| // Clear the unfinished message buffer. | ||
| this._unfinishedMessage = new Buffer(0); |
There was a problem hiding this comment.
new Buffer is deprecated
https://nodejs.org/api/buffer.html#buffer_new_buffer_size
package.json
Outdated
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "name": "ios-device-lib", | |||
| "version": "0.1.0", | |||
| "version": "0.1.1", | |||
There was a problem hiding this comment.
breaking changes should increase the minor version when your version is 0.x.x
rosen-vladimirov
approved these changes
Feb 14, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When we get the device log the C++ code starts to print on the stdout and in the JavaScript code we listen for the data event. If there is enough time between each message which is printed on the stdout the data event is raised with only one message. If the C++ code starts to print on the stdout too fast (which is the case with the device log) NodeJS buffers all the data and emits data event with all the buffered data. This breaks out code because we expect only one message. The solution for this problem is to write method which will "parse" the buffered data. This method should check the header of the received buffered data and read message from the buffered data. If there is full message in the buffered data we will read it. If there is no full message we should store the unfinished message and on the next received buffered data we should concat the unfinished message and the new data and try to parse the concatenated message. We rely on that the first message should have header with length. After that the method will make sure that the unfinished message will always begin with header with length and that each recursive call will receive data which begins with header with length.
While testing the new method with 7 threads which print on the stdout we noticed that at some point the data which is printed on the stdout gets malformed. For example with this message:
{"message1":"Feb 10 13:59: 28 dragon - iPhone6 timed[65] <Notice> : (Error)TimeLink : B76E090A - 9807 - 4AA3 - BB01 - 3CDE3DD48D55(4 \n- TMLSSourceDevice) send failure(Error Domain = com.apple.identityservices.error Code = 23 UserInfo=0x12fe326f0 {NSLocalizedDescription=Timed out, NSUnderlyingError=0x12fe34190 The operation couldnt be completed. (com.apple.ids.idssenderrordomain error 12.)}), will retry"}we receive random number of messages before we receive one message with additional
}. With one thread we do not receive malformed output. To solve this problem we need to add mutex in the print method and make it thread safe.