Fixes and hardening #720

Merged
merged 150 commits into from Jul 29, 2016

Conversation

Projects
None yet
8 participants
@tompaana
Member

tompaana commented Apr 27, 2016

  • Set a custom manufacturer ID for BLE advertisements
  • Fixed #659
  • Fixed #657
  • Fixed #642

This change is Reviewable

@msftclas

This comment has been minimized.

Show comment
Hide comment
@msftclas

msftclas Apr 27, 2016

Hi @tompaana, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!


It looks like you're a Microsoft contributor (Paananen Tomi (DX/Tampere)). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

msftclas commented Apr 27, 2016

Hi @tompaana, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!


It looks like you're a Microsoft contributor (Paananen Tomi (DX/Tampere)). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

@ThaliTester

This comment has been minimized.

Show comment
Hide comment
@ThaliTester

ThaliTester Apr 27, 2016

Member

PR is added to the queue for testing as 1. task. (efa6102)

Member

ThaliTester commented Apr 27, 2016

PR is added to the queue for testing as 1. task. (efa6102)

@yaronyg yaronyg added the in progress label Apr 27, 2016

@tompaana tompaana added this to the New Infra milestone Apr 27, 2016

@tompaana tompaana added the 2 - Ready label Apr 27, 2016

@ThaliTester

This comment has been minimized.

Show comment
Hide comment

@yaronyg yaronyg added 3 - Working and removed 2 - Ready labels Apr 28, 2016

@yaronyg

This comment has been minimized.

Show comment
Hide comment
@yaronyg

yaronyg Apr 28, 2016

Member

I'm out of time for tonight and I want to take my time with Streamcopyingthread and socketthreadbase per our previously conversation. So I'll review them tomorrow.


Reviewed 9 of 11 files at r1.
Review status: 9 of 11 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


src/android/java/io/jxcore/node/StartStopOperationHandler.java, line 221 [r1] (raw file):
Shouldn't overflow go to 0 not 1?


test/www/jxcore/bv_tests/testThaliMobileNativeWrapper.js, line 714 [r1] (raw file):
/ran/run


test/www/jxcore/bv_tests/testThaliMobileNativeWrapper.js, line 716 [r1] (raw file):
This shouldn't be possible. listenerRecreatedAfterFailure is what triggers us to fire the new nonTCPPeerAvailabilityChangedEvent. Go look at the function listenerRecreatedAfterFailureHandler in thaliMobileNativeWrapper. So something else isn't going right here. We need to understand what.


Comments from Reviewable

Member

yaronyg commented Apr 28, 2016

I'm out of time for tonight and I want to take my time with Streamcopyingthread and socketthreadbase per our previously conversation. So I'll review them tomorrow.


Reviewed 9 of 11 files at r1.
Review status: 9 of 11 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


src/android/java/io/jxcore/node/StartStopOperationHandler.java, line 221 [r1] (raw file):
Shouldn't overflow go to 0 not 1?


test/www/jxcore/bv_tests/testThaliMobileNativeWrapper.js, line 714 [r1] (raw file):
/ran/run


test/www/jxcore/bv_tests/testThaliMobileNativeWrapper.js, line 716 [r1] (raw file):
This shouldn't be possible. listenerRecreatedAfterFailure is what triggers us to fire the new nonTCPPeerAvailabilityChangedEvent. Go look at the function listenerRecreatedAfterFailureHandler in thaliMobileNativeWrapper. So something else isn't going right here. We need to understand what.


Comments from Reviewable

Merge remote-tracking branch 'origin/vNext' into vNext_tompaana_multi…
…ple_issues

# Conflicts:
#	thali/package.json
@ThaliTester

This comment has been minimized.

Show comment
Hide comment
@ThaliTester

ThaliTester Apr 28, 2016

Member

PR is added to the queue for testing as 1. task. (b92179b)

Member

ThaliTester commented Apr 28, 2016

PR is added to the queue for testing as 1. task. (b92179b)

@ThaliTester

This comment has been minimized.

Show comment
Hide comment
Member

ThaliTester commented Apr 28, 2016

@ThaliTester

This comment has been minimized.

Show comment
Hide comment
@ThaliTester

This comment has been minimized.

Show comment
Hide comment
@ThaliTester

ThaliTester Apr 28, 2016

Member

PR is added to the queue for testing as 1. task. (aff19f4)

Member

ThaliTester commented Apr 28, 2016

PR is added to the queue for testing as 1. task. (aff19f4)

@ThaliTester

This comment has been minimized.

Show comment
Hide comment
Member

ThaliTester commented Apr 28, 2016

@ThaliTester

This comment has been minimized.

Show comment
Hide comment
@ThaliTester

This comment has been minimized.

Show comment
Hide comment
@ThaliTester

ThaliTester Apr 28, 2016

Member

PR is added to the queue for testing as 1. task. (7227906)

Member

ThaliTester commented Apr 28, 2016

PR is added to the queue for testing as 1. task. (7227906)

@ThaliTester

This comment has been minimized.

Show comment
Hide comment
Member

ThaliTester commented Apr 28, 2016

@ThaliTester

This comment has been minimized.

Show comment
Hide comment
@ThaliTester

This comment has been minimized.

Show comment
Hide comment
@ThaliTester

ThaliTester Apr 29, 2016

Member

PR is added to the queue for testing as 1. task. (f73255a)

Member

ThaliTester commented Apr 29, 2016

PR is added to the queue for testing as 1. task. (f73255a)

@ThaliTester

This comment has been minimized.

Show comment
Hide comment
Member

ThaliTester commented Apr 29, 2016

@ThaliTester

This comment has been minimized.

Show comment
Hide comment
@yaronyg

This comment has been minimized.

Show comment
Hide comment
@yaronyg

yaronyg May 4, 2016

Member

Reviewed 1 of 1 files at r2, 1 of 3 files at r3, 3 of 3 files at r5.
Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.


src/android/java/io/jxcore/node/SocketThreadBase.java, line 173 [r5] (raw file):
This just makes me sad. The model is wrong. Fundamentally the two separate stream copying instances have their own separate life cycles (unless the parent socket is closed). They each are closed separately. :( This binds them together and prays hard that 1 second is enough to clean up a mess. Sigh...

I also wonder if the same delay shouldn't apply to onStreamCopyError? I honestly am not sure if there is a scenario where we would get an error on one stream (e.g. an exception, not a -1) and the other stream would still be viable.


src/android/java/io/jxcore/node/StreamCopyingThread.java, line 120 [r5] (raw file):
Shouldn't both totalNumberOfBytesRead and totalNumberOfbytesWritten be commented out when we aren't using the associated logging statements? No need to put arithmetic operations in a tight loop?


src/android/java/io/jxcore/node/StreamCopyingThread.java, line 136 [r5] (raw file):
Why is the if statement inside of the try/catch?


src/android/java/io/jxcore/node/StreamCopyingThread.java, line 145 [r5] (raw file):
This condition can't happen. If numberOfBytesRead is set in the while loop and if -1 the loop exits. If there was an exception from the read this condition still couldn't happen because numberOfBytesRead would not have been set. I think you can just remove this case since getting a -1 is anyway not an error and is handled properly by the if statement after the while


src/android/java/io/jxcore/node/StreamCopyingThread.java, line 161 [r5] (raw file):
Wouldn't it be safer to put this call inside of the catch?


src/android/java/io/jxcore/node/StreamCopyingThread.java, line 174 [r5] (raw file):
If close is called while a read request is outstanding then onStreamCopyingThreadDone will never get called because isDone will never be set to true. Is that intended?


src/android/java/io/jxcore/node/StreamCopyingThread.java, line 183 [r5] (raw file):
Why is this public? Testing? Because the production code only calls this from close() and calling closeStreams() without calling close (and hence setting mDoStop to true) seems bad. In fact, why aren't close and closeStreams just collapsed into a single method?


Comments from Reviewable

Member

yaronyg commented May 4, 2016

Reviewed 1 of 1 files at r2, 1 of 3 files at r3, 3 of 3 files at r5.
Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.


src/android/java/io/jxcore/node/SocketThreadBase.java, line 173 [r5] (raw file):
This just makes me sad. The model is wrong. Fundamentally the two separate stream copying instances have their own separate life cycles (unless the parent socket is closed). They each are closed separately. :( This binds them together and prays hard that 1 second is enough to clean up a mess. Sigh...

I also wonder if the same delay shouldn't apply to onStreamCopyError? I honestly am not sure if there is a scenario where we would get an error on one stream (e.g. an exception, not a -1) and the other stream would still be viable.


src/android/java/io/jxcore/node/StreamCopyingThread.java, line 120 [r5] (raw file):
Shouldn't both totalNumberOfBytesRead and totalNumberOfbytesWritten be commented out when we aren't using the associated logging statements? No need to put arithmetic operations in a tight loop?


src/android/java/io/jxcore/node/StreamCopyingThread.java, line 136 [r5] (raw file):
Why is the if statement inside of the try/catch?


src/android/java/io/jxcore/node/StreamCopyingThread.java, line 145 [r5] (raw file):
This condition can't happen. If numberOfBytesRead is set in the while loop and if -1 the loop exits. If there was an exception from the read this condition still couldn't happen because numberOfBytesRead would not have been set. I think you can just remove this case since getting a -1 is anyway not an error and is handled properly by the if statement after the while


src/android/java/io/jxcore/node/StreamCopyingThread.java, line 161 [r5] (raw file):
Wouldn't it be safer to put this call inside of the catch?


src/android/java/io/jxcore/node/StreamCopyingThread.java, line 174 [r5] (raw file):
If close is called while a read request is outstanding then onStreamCopyingThreadDone will never get called because isDone will never be set to true. Is that intended?


src/android/java/io/jxcore/node/StreamCopyingThread.java, line 183 [r5] (raw file):
Why is this public? Testing? Because the production code only calls this from close() and calling closeStreams() without calling close (and hence setting mDoStop to true) seems bad. In fact, why aren't close and closeStreams just collapsed into a single method?


Comments from Reviewable

@evabishchevich

This comment has been minimized.

Show comment
Hide comment
@evabishchevich

evabishchevich Jul 25, 2016

Member

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


src/android/java/io/jxcore/node/StartStopOperationHandler.java, line 221 [r1] (raw file):

Previously, yaronyg (Yaron Y Goland) wrote…

This comment still applies I believe, shouldn't extraInformation = 0 when overflowing? Otherwise aren't we losing one of our change values? E.g. we will only use 1..255 instead of 0.255?

Tomi used 1 instead of 0 because there is the constant NO_EXTRA_INFORMATION = 0 in PeerProperties in BtLibrary. 0 looks like reserved value

Comments from Reviewable

Member

evabishchevich commented Jul 25, 2016

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


src/android/java/io/jxcore/node/StartStopOperationHandler.java, line 221 [r1] (raw file):

Previously, yaronyg (Yaron Y Goland) wrote…

This comment still applies I believe, shouldn't extraInformation = 0 when overflowing? Otherwise aren't we losing one of our change values? E.g. we will only use 1..255 instead of 0.255?

Tomi used 1 instead of 0 because there is the constant NO_EXTRA_INFORMATION = 0 in PeerProperties in BtLibrary. 0 looks like reserved value

Comments from Reviewable

@yaronyg

This comment has been minimized.

Show comment
Hide comment
@yaronyg

yaronyg Jul 25, 2016

Member

@evabishchevich Make sure you merge this with #762

Member

yaronyg commented Jul 25, 2016

@evabishchevich Make sure you merge this with #762

jareksl and others added some commits Jul 26, 2016

Merge pull request #792 from jareksl/vNext_jareksl_555_clean
Switched off execution of the node tests.
Merge pull request #793 from jareksl/vNext_jareksl_555_clean
Changed build target (only android)
Merge pull request #794 from jareksl/vNext_jareksl_555_clean
Commented out compilation for iOS
Merge pull request #796 from jareksl/vNext_jareksl_555_clean
Commented out copy of app for iOS
Eugene Vabishchevich
Merge branch 'remotes/origin/vNext_jareksl_555_clean' into vNext_tomp…
…aana_multiple_issues

# Conflicts:
#	thali/package.json
Fixed some tests, added toString test in StartStop operation
@ThaliTester

This comment has been minimized.

Show comment
Hide comment
@ThaliTester

ThaliTester Jul 27, 2016

Member

PR is added to the queue for testing as 2. task. (3e34e91)

Member

ThaliTester commented Jul 27, 2016

PR is added to the queue for testing as 2. task. (3e34e91)

@ThaliTester

This comment has been minimized.

Show comment
Hide comment
@evabishchevich

This comment has been minimized.

Show comment
Hide comment
@evabishchevich

evabishchevich Jul 27, 2016

Member

Review status: 11 of 50 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


src/android/java/io/jxcore/node/OutgoingSocketThread.java, line 81 [r7] (raw file):

Previously, yaronyg (Yaron Y Goland) wrote…

How exactly would we end up with a null pointer exception?

There is no possibility to get NPE

Comments from Reviewable

Member

evabishchevich commented Jul 27, 2016

Review status: 11 of 50 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


src/android/java/io/jxcore/node/OutgoingSocketThread.java, line 81 [r7] (raw file):

Previously, yaronyg (Yaron Y Goland) wrote…

How exactly would we end up with a null pointer exception?

There is no possibility to get NPE

Comments from Reviewable

@yaronyg

This comment has been minimized.

Show comment
Hide comment
@yaronyg

yaronyg Jul 27, 2016

Member

@evabishchevich I'm going to hold off on reviewing this until 555 is checked in. Most of the diffs here are caused by your merge with 555 and so I would just be reviewing the same changes twice. Instead we'll wait for 555 to go into vNext and then this should only show changes post 555.


Reviewed 1 of 39 files at r11.
Review status: 12 of 50 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


src/android/java/io/jxcore/node/StartStopOperationHandler.java, line 221 [r1] (raw file):

Previously, evabishchevich wrote…

Tomi used 1 instead of 0 because there is the constant NO_EXTRA_INFORMATION = 0 in PeerProperties in BtLibrary. 0 looks like reserved value

Reserved for what purpose? Also, how can we just delete this code? How do we deal with state roll over?

Comments from Reviewable

Member

yaronyg commented Jul 27, 2016

@evabishchevich I'm going to hold off on reviewing this until 555 is checked in. Most of the diffs here are caused by your merge with 555 and so I would just be reviewing the same changes twice. Instead we'll wait for 555 to go into vNext and then this should only show changes post 555.


Reviewed 1 of 39 files at r11.
Review status: 12 of 50 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


src/android/java/io/jxcore/node/StartStopOperationHandler.java, line 221 [r1] (raw file):

Previously, evabishchevich wrote…

Tomi used 1 instead of 0 because there is the constant NO_EXTRA_INFORMATION = 0 in PeerProperties in BtLibrary. 0 looks like reserved value

Reserved for what purpose? Also, how can we just delete this code? How do we deal with state roll over?

Comments from Reviewable

Eugene Vabishchevich
@ThaliTester

This comment has been minimized.

Show comment
Hide comment
@ThaliTester

ThaliTester Jul 28, 2016

Member

PR is added to the queue for testing as 3. task. (40493cb)

Member

ThaliTester commented Jul 28, 2016

PR is added to the queue for testing as 3. task. (40493cb)

Eugene Vabishchevich
@ThaliTester

This comment has been minimized.

Show comment
Hide comment
@ThaliTester

ThaliTester Jul 28, 2016

Member

PR is added to the queue for testing as 3. task. (ea857b4)

Member

ThaliTester commented Jul 28, 2016

PR is added to the queue for testing as 3. task. (ea857b4)

@ThaliTester

This comment has been minimized.

Show comment
Hide comment
Member

ThaliTester commented Jul 28, 2016

@ThaliTester

This comment has been minimized.

Show comment
Hide comment
@ThaliTester

ThaliTester Jul 28, 2016

Member

Test 6810213040493cb(40493cb) has successfully finished without an error

See https://github.com/ThaliTester/TestResults/tree/6810213040493cb_Fixes_and_hardening_tompaana/ for the logs

Member

ThaliTester commented Jul 28, 2016

Test 6810213040493cb(40493cb) has successfully finished without an error

See https://github.com/ThaliTester/TestResults/tree/6810213040493cb_Fixes_and_hardening_tompaana/ for the logs

@ThaliTester

This comment has been minimized.

Show comment
Hide comment
Member

ThaliTester commented Jul 28, 2016

@ThaliTester

This comment has been minimized.

Show comment
Hide comment
@ThaliTester

ThaliTester Jul 28, 2016

Member

Test 68102130ea857b4(ea857b4) has successfully finished without an error

See https://github.com/ThaliTester/TestResults/tree/68102130ea857b4_Fixes_and_hardening_tompaana/ for the logs

Member

ThaliTester commented Jul 28, 2016

Test 68102130ea857b4(ea857b4) has successfully finished without an error

See https://github.com/ThaliTester/TestResults/tree/68102130ea857b4_Fixes_and_hardening_tompaana/ for the logs

@yaronyg

This comment has been minimized.

Show comment
Hide comment
@yaronyg

yaronyg Jul 29, 2016

Member

@evabishchevich There is just one open issue left and it's really a question. Once we have settled that question then consider this good to check in.


Reviewed 23 of 39 files at r11, 4 of 27 files at r12, 22 of 23 files at r13, 1 of 1 files at r14.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


src/android/java/io/jxcore/node/StartStopOperationHandler.java, line 221 [r1] (raw file):

Previously, yaronyg (Yaron Y Goland) wrote…

Reserved for what purpose? Also, how can we just delete this code? How do we deal with state roll over?

I still need to understand what purpose Tomi was using the 0 for and why he couldn't set extraInformation to 0. Please don't check in until we have this fully clarified.

Comments from Reviewable

Member

yaronyg commented Jul 29, 2016

@evabishchevich There is just one open issue left and it's really a question. Once we have settled that question then consider this good to check in.


Reviewed 23 of 39 files at r11, 4 of 27 files at r12, 22 of 23 files at r13, 1 of 1 files at r14.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


src/android/java/io/jxcore/node/StartStopOperationHandler.java, line 221 [r1] (raw file):

Previously, yaronyg (Yaron Y Goland) wrote…

Reserved for what purpose? Also, how can we just delete this code? How do we deal with state roll over?

I still need to understand what purpose Tomi was using the 0 for and why he couldn't set extraInformation to 0. Please don't check in until we have this fully clarified.

Comments from Reviewable

@yaronyg yaronyg removed their assignment Jul 29, 2016

@yaronyg yaronyg merged commit a6bbc33 into vNext Jul 29, 2016

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
code-review/reviewable 2 discussions left (evabishchevich, yaronyg)
Details

@yaronyg yaronyg removed the in progress label Jul 29, 2016

@evabishchevich evabishchevich deleted the vNext_tompaana_multiple_issues branch Feb 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment