dkrprasetya / simple-firebase-unity

Firebase Realtime-Database's REST API Wrapper for Unity in C#
143 stars 40 forks source link

HttpRequests are now asynchronous + removed use of Reflection #1

Closed Whyser closed 8 years ago

Whyser commented 8 years ago

As the title says. I've made each HttpRequest (Get, Push, Delete and Set) method asynchronous (.NET 3.5 style, so should still be compatible with Unity). I see no reason for it to be synchronous since the data is retrieved via callbacks anyway. I have absolutely no idea why you used reflection to access the "Headers" field instead of setting it through it's property, I removed the use of Reflection.

I haven't tested this on actual devices other than in the Unity Editor (where it seems to work). You should do some testing before updating the Unity Store Assets if you intend to merge this Pull Request. :)

(EDIT: Might need to do a bit more to make it "truly" async (see answer: http://stackoverflow.com/a/13963255/2422321))

dkrprasetya commented 8 years ago

Thanks for the contribution Whyser! I will check your changes later today.

At first I thought that some people would be lazy enough to implement the synchronous one by themselves while making it async in Unity is as simple as using Coroutines isn't? But good point though :D

For the reflection, I forgot precisely why (I made this quite a while ago actually), but in the version of Android or iOS or something, somehow it doesn't work without using reflection.

I'll update again after checking your code and testing it.

Again, thanks for contributing!

Whyser commented 8 years ago

The Coroutines aren't really asynchronous. They run on the same thread and thus will cause hiccup in the app. In my case a freeze for about 0.14-0.5 seconds. :)

I noticed however that the callback is not invoked on the main thread which causes error if trying to touch Unity-related stuff (transform, GetComponent etc).

I will see if I can find a solution. :)

EDIT: I can't seem to think of a solution that doesn't involve "ugliness", System.Threading or System.Threading.Tasks which would complicate stuff (for cross-platform). One way is to queue the events on the Unity-thread, but that would require something like: Firebase.Update() which is called inside an MonoBeheviour Update-loop which is ugly. Maybe this pull request isn't good after all. :(

EDIT (again): If you are not scared of a UnityEngine.dll dependency. It would be possible to create a GameObject and use it's update loop to dispatch events to the "main thread". Tell me if you are interested and I'll make a new pull request.

Whyser commented 8 years ago

Another issue:

Apparently your use of some WebRequest-features is not compatible with Windows Universal apps (UWP), I've simplified some code and removed code which I'm not sure did anything at all (although I'm uncertain because I'm no expert with HttpRequests).

For example: In FirebaseRoot.cs: You make use of System.Net.Security namespace, and do some stuff inside FirebaseRoot constructor, but as far as I can see, that code don't actually do anything?

Also, your "special class" CusteredHeaderCollection which inherits from WebHeaderCollection will not be possible in UWP (Universal Apps) because WenHeaderCollection is a sealed class at that platform. But, I don't really see any use of this class either. The only thing it does is set the "Host" parameter of a WebHeaderCollection. I tried setting the "Host" parameter directly but the application failed to continue when I did. But not setting it at all seems to work.

If any of this code is actually necessary, mind explaining to me how they are used? :)

@dkrprasetya

Whyser commented 8 years ago

I'm closing this Pull Request as it has too many errors. I'll make a fork based on your code but it will be using Unity's WWW requests instead to make it "truly" cross-platform.

dkrprasetya commented 8 years ago

Hi Wyser, sorry for responding very late! It has been a rough week... 💀

I'm also not an expert btw :( , but I'll try my best to explain! CMIIW

When accessing HTTPS or SSL, there are cases where we need to handle the certificate verification errors, whether it is unknown certificate authority or etc. In our case, on Android if we didn't handle it, the connection to Firebase will be denied. System.Net.Security namespace is used to access class SslPolicyErrors. For reading references maybe you could see:

For the Host parameter, that time when I developed this on the first time I learnt that you need to set the host value manually to assure that it sends the exact same working request when you do a simple REST request to Firebase endpoint, and by trial & error, I found out sometimes there were errors on some platform (forgot which it is) when you didn't do that.

I did tests again though recently (on Windows desktop, OSX, Android, and iOS), it seems that it still works without assigning host header manually. Need to do more tests to make it sure 100%. I will inform you later about my findings on this!

About your findings on Async part, looks like you're right, from what I found Coroutines are not purely Async :O

Thanks for the experiments though. I also currently trying to find the "prettier" way to make it purely Async. If you like, I will share it to you when I found out cool alternatives. I don't think creating a new GameObject will make a significant performance improvement, and it is not pretty! lol. For the mean time I think it is best to keep it simple as it is now.

dkrprasetya commented 8 years ago

I learnt a ton from you though, thanks a lot Wyser! :D

Thanks to you we also learnt that our current code (especially the "host assign" part) doesn't work on UWP.

For the mean time (before I made it sure 100% that this "host assign" part is unnecessary), maybe we can make a work around by surrounding the "host assign" part with a try-catch but without any necessary handling action, as exception looks like only happens on UWP and apparently it still works perfectly nevertheless, right? Or maybe using #if macros to skip the "host assign" part for UWP cases.

Maybe, could you help me with the implementation and then do tests on UWP platforms as you seems to have access to them? Looking forward to hear from you!

Whyser commented 8 years ago

I tried to get it to work on UWP, but didn't manage because the ContentLength-field on the WebRequest object is not available for UWP apps it seems. And it's impossible to set the Content-Length header because it will throw an exception.

Therefore I went with re-creating the plugin using Unitys WWW-classes and using the "X-Http-Method-Override" header to set the method (GET, PUT, PATCH and DELETE, luckily Firebase seems to support this non-standard header). This seems to work on Android, UWP and the Editor, iOS (Desktop and Web player has yet to be tested, but should work since Unitys WWW should work out-of-the-box).

It's now dependant on UnityEngine.dll though, but that's fine with me, since my use-case is only for Unity.