Cle fixes #151

Merged
merged 19 commits into from Oct 21, 2015

Conversation

Projects
None yet
2 participants
@tobybrad
Contributor

tobybrad commented Oct 13, 2015

Two issues fixed:

  • Sending wrong peer identifier in connect invite. Not an issue when you have just 1 other device in range, big one if you don't
  • Not being called after first notification of data to read on an input stream. Bizarrely not exhausting the input stream on that first read fixes the problem and you'll get repeatedly called as long as there are still bytes available. This is almost certainly a bug in Apple's Framework and worthy of further investigation but let's get this working first.

Other fixes relating to the client server socket are to ensure that the upper layer (e.g. a multiplexer) can close and re-open it's bridged socket (seems reasonable to allow).

Third issue just identified :-(... sending a large file (like a photo) overwhelms the output buffer. Fixes to follow. Fixed !!

Review on Reviewable

@yaronyg yaronyg added the in progress label Oct 13, 2015

@yaronyg yaronyg self-assigned this Oct 13, 2015

@yaronyg yaronyg added this to the 0.0.1 - Identity Exchange milestone Oct 13, 2015

@yaronyg

This comment has been minimized.

Show comment
Hide comment
@yaronyg

yaronyg Oct 13, 2015

Member

Reviewed 12 of 13 files at r2.
Review status: all files reviewed at latest revision, 8 unresolved discussions, all commit checks successful.


src/ios/MultipeerConnectivity/THEMultipeerClientSocketRelay.m, line 77 [r2] (raw file):
Shouldn't we at least log the error?


src/ios/MultipeerConnectivity/THEMultipeerSocketRelay.m, line 120 [r2] (raw file):
If we are going to allow for reconnect then at a minimum we have to have a test for it in the emitter tests that shows it works properly. But I'd be happy to just see this capability pulled out. If the socket is disconnected for any reason then we can just declare ourselves in a bad state and require a new connection request. It's simpler and requires less testing code. :)


src/ios/MultipeerConnectivity/THEMultipeerSocketRelay.m, line 199 [r2] (raw file):
Since the assert isn't in the synchronized isn't it at least theoretically possible for _socket t be changed between the time the function was called and when the assert check is made?


src/ios/MultipeerConnectivity/THEMultipeerSocketRelay.m, line 206 [r2] (raw file):
It is possible for writeOutputStream to return NO, especially if there is an error. But right now, if I'm reading this correctly, if we get that NO and are in a production build then we will silently swallow it. Shouldn't we at least declare the stream to be invalid and close down the native socket thus forcing the emitter to open a new one and letting us reset our state?


src/ios/MultipeerConnectivity/THEMultipeerSocketRelay.m, line 233 [r2] (raw file):
Since _socket is accessed by code that is using synchronized(self) doesn't that mean that this code should also use synchronized(self) to prevent any possibility of state conflict? Or is socketDidDisconnect somehow guaranteed to only be called when no other methods are in progress?


src/ios/MultipeerConnectivity/THEMultipeerSocketRelay.m, line 313 [r2] (raw file):
This is related to my previous comment above on writeOutputStream. If writeOutputStream returns NO because of an error don't we need to tear everything down?


test/www/jxcore/bv_tests/testThaliNativeLayer.js, line 204 [r2] (raw file):
You call that large?!?!?! Shesh... How about at least a few meg?


test/www/jxcore/bv_tests/testThaliReplicationManager.js, line 210 [r2] (raw file):
Seriously, that isn't a large blob. I would try at least 10 meg. No, I'm not kidding. We should be pushing the limits in our tests.


Comments from the review on Reviewable.io

Member

yaronyg commented Oct 13, 2015

Reviewed 12 of 13 files at r2.
Review status: all files reviewed at latest revision, 8 unresolved discussions, all commit checks successful.


src/ios/MultipeerConnectivity/THEMultipeerClientSocketRelay.m, line 77 [r2] (raw file):
Shouldn't we at least log the error?


src/ios/MultipeerConnectivity/THEMultipeerSocketRelay.m, line 120 [r2] (raw file):
If we are going to allow for reconnect then at a minimum we have to have a test for it in the emitter tests that shows it works properly. But I'd be happy to just see this capability pulled out. If the socket is disconnected for any reason then we can just declare ourselves in a bad state and require a new connection request. It's simpler and requires less testing code. :)


src/ios/MultipeerConnectivity/THEMultipeerSocketRelay.m, line 199 [r2] (raw file):
Since the assert isn't in the synchronized isn't it at least theoretically possible for _socket t be changed between the time the function was called and when the assert check is made?


src/ios/MultipeerConnectivity/THEMultipeerSocketRelay.m, line 206 [r2] (raw file):
It is possible for writeOutputStream to return NO, especially if there is an error. But right now, if I'm reading this correctly, if we get that NO and are in a production build then we will silently swallow it. Shouldn't we at least declare the stream to be invalid and close down the native socket thus forcing the emitter to open a new one and letting us reset our state?


src/ios/MultipeerConnectivity/THEMultipeerSocketRelay.m, line 233 [r2] (raw file):
Since _socket is accessed by code that is using synchronized(self) doesn't that mean that this code should also use synchronized(self) to prevent any possibility of state conflict? Or is socketDidDisconnect somehow guaranteed to only be called when no other methods are in progress?


src/ios/MultipeerConnectivity/THEMultipeerSocketRelay.m, line 313 [r2] (raw file):
This is related to my previous comment above on writeOutputStream. If writeOutputStream returns NO because of an error don't we need to tear everything down?


test/www/jxcore/bv_tests/testThaliNativeLayer.js, line 204 [r2] (raw file):
You call that large?!?!?! Shesh... How about at least a few meg?


test/www/jxcore/bv_tests/testThaliReplicationManager.js, line 210 [r2] (raw file):
Seriously, that isn't a large blob. I would try at least 10 meg. No, I'm not kidding. We should be pushing the limits in our tests.


Comments from the review on Reviewable.io

@yaronyg yaronyg assigned tobybrad and unassigned yaronyg Oct 13, 2015

@yaronyg

This comment has been minimized.

Show comment
Hide comment
@yaronyg

yaronyg Oct 13, 2015

Member

Finished putting in comments. Ball is now in your court.

Member

yaronyg commented Oct 13, 2015

Finished putting in comments. Ball is now in your court.

@yaronyg

This comment has been minimized.

Show comment
Hide comment
@yaronyg

yaronyg Oct 15, 2015

Member

Toby is changing his behavior to match Jukka's which is that we only allow exactly one connection to the native port and if anything goes wrong you have to call disconnect and then connect. Also Jukka will throw an event if the p2p connection fails so the user knows that since we can't communicate that over the TCP/IP connection. This work should take 1/2 a day.

Member

yaronyg commented Oct 15, 2015

Toby is changing his behavior to match Jukka's which is that we only allow exactly one connection to the native port and if anything goes wrong you have to call disconnect and then connect. Also Jukka will throw an event if the p2p connection fails so the user knows that since we can't communicate that over the TCP/IP connection. This work should take 1/2 a day.

@yaronyg

This comment has been minimized.

Show comment
Hide comment
@yaronyg

yaronyg Oct 16, 2015

Member

This should be done in 1.5 hours.

Member

yaronyg commented Oct 16, 2015

This should be done in 1.5 hours.

@yaronyg

This comment has been minimized.

Show comment
Hide comment
@yaronyg

yaronyg Oct 19, 2015

Member

The cordova issue was a blocker and Oguz fixed it. Just bugs and bugs and bugs.

Member

yaronyg commented Oct 19, 2015

The cordova issue was a blocker and Oguz fixed it. Just bugs and bugs and bugs.

@tobybrad

This comment has been minimized.

Show comment
Hide comment
@tobybrad

tobybrad Oct 20, 2015

Contributor

Review status: 7 of 15 files reviewed at latest revision, 5 unresolved discussions, all commit checks successful.


src/ios/MultipeerConnectivity/THEMultipeerClientSocketRelay.m, line 77 [r2] (raw file):
Debatable that it's an error. You can take the view that the transport layer is tightly coupled to the mux layer in which case perhaps this would be an error. But if you wanted to keep these things loosely coupled then it may well be perfectly acceptable for the application layer to close the socket.

UPDATE: After talking to Jukka I understand his code tears down the p2p connection and sends a new event to indicate the connection has died. I'll change to match and submit a test as part of this PR to exercise it.


src/ios/MultipeerConnectivity/THEMultipeerSocketRelay.m, line 120 [r2] (raw file):
"declare ourselves in a bad state" - this'd require a new event.. which I understand now exists on a branch somewhere. I'll match android behaviour for the short term but I think we should consider changing long term to allow socket to disconnect/reconnect as the app sees fit.


src/ios/MultipeerConnectivity/THEMultipeerSocketRelay.m, line 206 [r2] (raw file):
Without a framing mechanism closing a socket when you've failed to write to an output stream isn't going to help (the remote side isn't going to see you close that socket). The only remedy (recently) available to us is to tear down the whole p2p connection. Also.. writeOutputStream returning NO indicate that there simply isn't any data send. We need to record this this fact to keep things running. Changed the error condition to throw exception instead.


test/www/jxcore/bv_tests/testThaliNativeLayer.js, line 204 [r2] (raw file):
Multipeer connectivity, particularly over my local wifi connection for some reason, is very slow to transfer on occasion. 128k adequately exercises the bridge without making me wait an even more miserable length of time than I really need to.


test/www/jxcore/bv_tests/testThaliReplicationManager.js, line 210 [r2] (raw file):
i really, really do not want to wait another 10 minutes on top of the install time every time I need to do a test run.


Comments from the review on Reviewable.io

Contributor

tobybrad commented Oct 20, 2015

Review status: 7 of 15 files reviewed at latest revision, 5 unresolved discussions, all commit checks successful.


src/ios/MultipeerConnectivity/THEMultipeerClientSocketRelay.m, line 77 [r2] (raw file):
Debatable that it's an error. You can take the view that the transport layer is tightly coupled to the mux layer in which case perhaps this would be an error. But if you wanted to keep these things loosely coupled then it may well be perfectly acceptable for the application layer to close the socket.

UPDATE: After talking to Jukka I understand his code tears down the p2p connection and sends a new event to indicate the connection has died. I'll change to match and submit a test as part of this PR to exercise it.


src/ios/MultipeerConnectivity/THEMultipeerSocketRelay.m, line 120 [r2] (raw file):
"declare ourselves in a bad state" - this'd require a new event.. which I understand now exists on a branch somewhere. I'll match android behaviour for the short term but I think we should consider changing long term to allow socket to disconnect/reconnect as the app sees fit.


src/ios/MultipeerConnectivity/THEMultipeerSocketRelay.m, line 206 [r2] (raw file):
Without a framing mechanism closing a socket when you've failed to write to an output stream isn't going to help (the remote side isn't going to see you close that socket). The only remedy (recently) available to us is to tear down the whole p2p connection. Also.. writeOutputStream returning NO indicate that there simply isn't any data send. We need to record this this fact to keep things running. Changed the error condition to throw exception instead.


test/www/jxcore/bv_tests/testThaliNativeLayer.js, line 204 [r2] (raw file):
Multipeer connectivity, particularly over my local wifi connection for some reason, is very slow to transfer on occasion. 128k adequately exercises the bridge without making me wait an even more miserable length of time than I really need to.


test/www/jxcore/bv_tests/testThaliReplicationManager.js, line 210 [r2] (raw file):
i really, really do not want to wait another 10 minutes on top of the install time every time I need to do a test run.


Comments from the review on Reviewable.io

@yaronyg yaronyg assigned yaronyg and unassigned tobybrad Oct 20, 2015

@yaronyg

This comment has been minimized.

Show comment
Hide comment
@yaronyg

yaronyg Oct 20, 2015

Member

Reviewed 1 of 4 files at r4, 2 of 2 files at r5, 5 of 6 files at r6.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


src/ios/MultipeerConnectivity/THEMultipeerClientSession.m, line 147 [r6] (raw file):
Wouldn't it be easier to just create the JSON as a string (since it is static and small) and thus avoid even the possibility of error being nill?


src/ios/THEAppContext.m, line 126 [r6] (raw file):
A comment explaining that we are disabling BLE for now as we are only focused on working on iOS in the foreground would seem appropriate.


Comments from the review on Reviewable.io

Member

yaronyg commented Oct 20, 2015

Reviewed 1 of 4 files at r4, 2 of 2 files at r5, 5 of 6 files at r6.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


src/ios/MultipeerConnectivity/THEMultipeerClientSession.m, line 147 [r6] (raw file):
Wouldn't it be easier to just create the JSON as a string (since it is static and small) and thus avoid even the possibility of error being nill?


src/ios/THEAppContext.m, line 126 [r6] (raw file):
A comment explaining that we are disabling BLE for now as we are only focused on working on iOS in the foreground would seem appropriate.


Comments from the review on Reviewable.io

@tobybrad

This comment has been minimized.

Show comment
Hide comment
@tobybrad

tobybrad Oct 21, 2015

Contributor

Review status: 14 of 15 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


src/ios/MultipeerConnectivity/THEMultipeerClientSession.m, line 147 [r6] (raw file):
I don't know what peerIdentifier might be in practice, could require special escaping rules whatever.. safer to go this way.


Comments from the review on Reviewable.io

Contributor

tobybrad commented Oct 21, 2015

Review status: 14 of 15 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


src/ios/MultipeerConnectivity/THEMultipeerClientSession.m, line 147 [r6] (raw file):
I don't know what peerIdentifier might be in practice, could require special escaping rules whatever.. safer to go this way.


Comments from the review on Reviewable.io

tobybrad added a commit that referenced this pull request Oct 21, 2015

@tobybrad tobybrad merged commit f63e1d8 into story_001_customer Oct 21, 2015

1 check passed

code-review/gitcolony 0/0 qa 0/0 dev, 0 issues
Details

@tobybrad tobybrad removed the in progress label Oct 21, 2015

@tobybrad tobybrad deleted the cle_fixes branch Oct 21, 2015

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