Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TIMOB-26030] Android: Fixed TCP read()/write() to not crash when JS running on main thread #10338

Merged
merged 3 commits into from Sep 24, 2018

Conversation

jquick-axway
Copy link
Contributor

@jquick-axway jquick-axway commented Sep 24, 2018

JIRA:
https://jira.appcelerator.org/browse/TIMOB-26030

Note:
Unit test already exists for this in mocha suite. It's been failing on Android the whole time.

Test:

  1. Open the project's "tiapp.xml" file.
  2. Make sure property "run-on-main-thread" is set to true.
  3. Set up an Android device with Internet access.
  4. Build and run TcpClientBlockingTest.js attached to TIMOB-26030.
  5. On app startup, verify that an HTTP response is shown onscreen.
  6. Back out of the app.
  7. Disconnect the device from the Internet.
  8. Restart the app.
  9. Verify that you see an "Unable to connect" message in the JSON output onscreen.
  10. Open "tiapp.xml" file and change property "run-on-main-thread" to false.
  11. Repeat steps 3-9.

@build
Copy link
Contributor

build commented Sep 24, 2018

Messages
📖

💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

Copy link
Contributor

@sgtcoolguy sgtcoolguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to retain the assignment/re-use of the inputStream field, but otherwise LGTM.

This is approximately 1000x easier/more elegant than what I did. I do think however, that we should consider deprecating the sync APIs (maybe?) or at the very least extending them to accept a callback function to operate async (it is very, very odd to me that we have an async variant on some separate utility proxy in a different namespace). But, that should be done in another PR. If this fixes the crash/blocking then it is a good fix that should be merged ASAP.

@jquick-axway
Copy link
Contributor Author

I agree 100% that the blocking read() and write() methods should be deprecated. Worst-case scenario, they'll block the thread for 1 minute (the default timeout) if you're behind a private network with no Internet access.

@lokeshchdhry
Copy link
Contributor

FR Passed.

read() and write() methods do not cause a crash & run as expected when run-on-main-thread = true.

Studio Ver: 5.1.1.201809051655
SDK Ver: 7.5.0 local build
OS Ver: 10.13.5
Xcode Ver: Xcode 9.4.1
Appc NPM: 4.2.13
Appc CLI: 7.0.6
Daemon Ver: 1.1.3
Ti CLI Ver: 5.1.1
Alloy Ver: 1.13.2
Node Ver: 8.9.1
NPM Ver: 5.5.1
Java Ver: 1.8.0_161
Devices: ⇨ google Nexus 5 (Android 6.0.1)
⇨ google Nexus 6P (Android 8.1.0)
Emulator: ⇨ Android 4.1.2

@lokeshchdhry lokeshchdhry merged commit de57da3 into tidev:master Sep 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants