search

TelemetryDeck / SwiftSDK

Swift SDK for TelemetryDeck, a privacy-conscious analytics service for apps and websites.
https://telemetrydeck.com/
Other
149 stars 31 forks source link

Wait For Connectivity Before Attempting Send #52

Closed Megatron1000 closed 9 months ago

Megatron1000 commented 2 years ago

I was just looking over the code and noticed it doesn't check whether the network is up before sending events.

It might be a good idea to configure the session with this property so, instead of blindly sending the request and it failing and then sticking it back in the queue to try again next time, it automatically sends them when the network becomes available.

https://developer.apple.com/documentation/foundation/nsurlsessionconfiguration/2908812-waitsforconnectivity

winsmith commented 2 years ago

Very nice point, thanks a lot for pointing that out! I'll see that I'll include that with the next update (if you feel like writing a PR, that's also very welcome)

ca13ra1 commented 2 years ago

@winsmith @Megatron1000 I've added it here, I can always submit a PR if needed. It wasn't much code added to support waitsForConnectivity for URLSession.

winsmith commented 2 years ago

Oh yes please!

ca13ra1 commented 2 years ago

@winsmith I had a small typo, forgot to remove shared from session, made a quick PR which solves the issue

finnvoor commented 2 years ago

I think this change drops signals that occur when a device is offline. Signals are popped and attempted to be sent, and if there is an error they are added back for sending later. With waitsForConnectivity = true, the session will not error when there is no connectivity, and signals will instead be lost if an app is quit before regaining connectivity. I guess there's also the chance that someone will quit the app in the middle of the URL request, also causing signals to be lost, though that is less likely (but definitely not impossible).

winsmith commented 2 years ago

Hmm that's not good! Investigating, thanks for the report!

Sherlouk commented 1 year ago

It might make more sense to revert the change for 'waitsForConnectivity' in favour of a Reachability based solution - this would allow us to receive a notification when an internet connection is restored and at that point we make a fresh attempt to pop items off the cache and post them to the server.

NWPathMonitor (https://developer.apple.com/documentation/network/nwpathmonitor) supports almost all the same versions as this library and is Apple's modern built-in solution.

winsmith commented 1 year ago

This sounds like a good idea. Originally I wanted to not do this because I was afraid it would block the SDK from running Linux based apps, but we do have a separate SDK for Vapor now, so that's no longer an issue.