Skip to content

Android support#12

Closed
koush wants to merge 3 commits intosocketio:masterfrom
koush:master
Closed

Android support#12
koush wants to merge 3 commits intosocketio:masterfrom
koush:master

Conversation

@koush
Copy link
Contributor

@koush koush commented Feb 28, 2019

Android uses an older version of org.json which throws exceptions on JSONObject.put. It never actually throws.
The change is to simply try/catch the JSONException and throw an assertion if anything is caught. I've literally never ever seen it thrown, so I'm unsure what would actually trigger it.
That exception seems to have been relaxed in newer versions of org.json.
It is not possible to replace the Android version of org.json via an external dependency. The framework version is always preferred.

The other change is to replace getOrDefault(key, null) with get, which is equivalent, and available on Android.
Android supports Java 8 language features on older runtimes (since it all compiled down to dalvik byte code), but not all the Java 8 libraries. getOrDefault is a Java8 method, and not available on Android.

Wrap with catch block for JSONException where necessary. Rethrow an assertion if caught, which should not happen.
Replace usage of getOrDefault(key, null) with get(key). They are equivalent. getOrDefault is only available on
Android N+.
@koush
Copy link
Contributor Author

koush commented Feb 28, 2019

This actually works fine, with no changes, on Android 26+, as getOrDefault is available. But the current recommended Android target is 21. The main necessary change is removing that. The org.json changes are only necessary if actually compiling using the Android toolchain. The runtime doesn't actually enforce exceptions in method signatures, but the compiler does.

I briefly looked at the Android client, and saw some potential Java 8 libraries in use that may not work on Android 21-25. But none of those usages are invoked by the server code.

@trinopoty
Copy link
Collaborator

This library was never meant to be used on anything other than servers.
@darrachequesne any comments on this PR?

@koush
Copy link
Contributor Author

koush commented Feb 28, 2019

@trinopoty I am using this library on an Android device that functions as a home automation server.

Quite a few Android devices serve as headless home hubs of some sort of another already, with a web interface, and there will be more.
It is a server, but not in the traditional (cloud) sense.
It's also not Android in the traditional Android consumer phone sense either; more Android Things (the embedded solution/platform).

The need for a real time two way channel exists. The current options are insecure websockets (websockets do not work with self signed local certificates in Safari mobile) or secure xhr polling with socket.io.

@trinopoty
Copy link
Collaborator

Other than the getOrDefault issue, is there anything else that refuses to run on Android?
Is wrapping the put calls in try-catch a necessity?
Can it be used by importing via gradle without the try-catch wrapping?

I'm asking because someone somewhere may forget that the wrapping is for Android and unwrap it.

@koush
Copy link
Contributor Author

koush commented Mar 1, 2019

The wrapped put calls are not necessary for running it on Android. But they are necessary for compiling it targeting Android using Android Studio. They can be removed, without affecting Android users.
I can add a comment that notes why the try/catches are necessary.

The Android linter did not find any other calls in the engine.io-server code would cause compat issues.

@trinopoty
Copy link
Collaborator

Can the jar be used with Android? With the Java 8 bytecode and all?
If so, it'd be better to unwrap the put statements.
Also, we need to check if get has the same behavior as getOrDefault(..., null).
Also, if the jar can be used as is, the build.gradle can also be removed.

@trinopoty
Copy link
Collaborator

Also, move the PR out of draft.

@koush
Copy link
Contributor Author

koush commented Mar 1, 2019

Yes, The Java 8 bytecode is transpiled to Android Dalvik byte code. That is not an issue.

I'd prefer to leave the wrapped statements in, as then further work for Android will require readding those wraps just to compile: I did want to follow up this pull request for a proposal to fix the aforementioned XHR-only denial of service issue.

Issue is moved out of draft. I didn't want it accidentally merged until discussion was complete.

@koush koush marked this pull request as ready for review March 1, 2019 19:17
@codecov-io
Copy link

Codecov Report

Merging #12 into master will decrease coverage by 1.41%.
The diff coverage is 58.33%.

@@            Coverage Diff             @@
##           master      #12      +/-   ##
==========================================
- Coverage   89.27%   87.85%   -1.42%     
==========================================
  Files          11       11              
  Lines         550      560      +10     
  Branches       84       84              
==========================================
+ Hits          491      492       +1     
- Misses         25       34       +9     
  Partials       34       34

@trinopoty
Copy link
Collaborator

I'm working on the XHR issue.
I'll re-evaluate this PR after that.

@trinopoty
Copy link
Collaborator

Both the async issue and the android getOrDefault to get have been addresses in the latest release.
Please check it out. Allow for a few hours for the release to become available on maven.

@koush
Copy link
Contributor Author

koush commented Mar 7, 2019

Trying to merge now and enable the async bit. Will take a bit, need to implement the new AsyncContext stuff.

@koush
Copy link
Contributor Author

koush commented Mar 7, 2019

I disabled websocket traffic, and implemented the necessary hooks. At first glance it all seems to be working.
scrypted_and_scrypted

@trinopoty
Copy link
Collaborator

Does it also solve the android runtime problems?

@koush
Copy link
Contributor Author

koush commented Mar 9, 2019

Let me reverify that there's no issues on the older versions of the Android runtime. Will get back to you shortly.

@koush
Copy link
Contributor Author

koush commented Mar 12, 2019

Seems to be working fine on older versions as well.

@koush koush closed this Mar 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants