feat: get current window screen size

[ioannisj]

💡 Motivation and Context

thread: #238

• Switch and block to main thread if needed to grab screen size
• Added a shared module extension to retrieve current window from UIApplication

#skip-changelog

💚 How did you test it?

📝 Checklist

• I reviewed the submitted code.
• I added tests to verify the changes.
• I updated the docs if needed.
• No breaking change or entry added to the changelog.

[dfed]

@ioannisj this .sync call creates a deadlock in my app. We wrap PostHogSDK.shared.isFeatureEnabled calls in a lock so that we can return stable values for a feature flag within one app lifecycle, like so:

	public func isEnabled(_ key: String) -> Bool {
		lock.sync {
			if let deliveredIsEnabled = unsafe_keyToPreviouslyDeliveredIsEnabled[key] {
				// Avoid potential bugs by always returning a stable value.
				// Do not allow a feature flag to change mid app lifecycle.
				return deliveredIsEnabled
			} else {
				let isEnabled = PostHogSDK.shared.isFeatureEnabled(key)
				unsafe_keyToPreviouslyDeliveredIsEnabled[key] = isEnabled
				return isEnabled
			}
		}
	}

On app launch, we call into this isEnabled(…) method from multiple queues, including main. If our background queue calls into this method just before main calls into this method, then the background queue gets the lock, the main queue waits on the lock, and then your code here waits forever for main to continue.

While this code exists in PostHog we will be unable to upgrade from our current version.

[dfed]

Ideally you wouldn't need to synchronously access this data just-in-time: a possible approach would be to intercept changes to the current window synchronously, capturing the window-specific values you need, and then storing those values in a locked container that background queues can access without getting a lock on all of main.

[ioannisj]

Hey @dfed thanx for raising this! Great oversight from our side that this could be potentially problematic. I'll take a look asap

[marandaneto]

maybe a simpler approach/quick workaround would be:

        if Thread.isMainThread {
            return getWindowSize()
        } else {
            var size: CGSize?
            let group = DispatchGroup()
            group.enter()
            DispatchQueue.main.async {
                size = getWindowSize()
                group.leave()
                
            }
            let timeout = DispatchTime.now() + .seconds(1)
            let _ = group.wait(timeout: timeout)
            return size
        }

or yes, listening to window changes and caching the values

[dfed]

That quick workaround would alleviate the deadlock but would cause a 1s hang on the main queue any time we would have deadlocked 😬

Certainly better than a deadlock, but also not something I can ship with.

Quickest fix imo is to cache the value behind a lock any time you're on main and read that cached value any time you're on a non-main queue. This would mean that you'd have to call into PostHog from main at least once before you'd get screen data attached to events, but that seems like a reasonable requirement.

[marandaneto]

If the SDK is init in a background thread, and the main thread is still holding a lock, the problem would persist though.

btw @dfed PostHogSDK.shared.isFeatureEnabled is already thread-safe, unless your app has to do many operations within that lock, but reading/writing a flag is thread-safe.
I understand your use case, but feels like the deadlock is caused by your own locks/logic, the main thread should be available since most features require main thread access, including our SDK and some of its features.
I agree that we should not contribute to a deadlock hence trying to figure out the best approach here but I'd not hold a lock in the main thread.

@ioannisj it's important to return the correct screen size info if we do caching, or better to not attach any data at all, I'd prioritize correct data over skewed data.

What's about this notification? Would that be reliable to observe and cache the current active window?

[ioannisj]

@marandaneto yeah, already on that currently (along with some other relevant notifications). For now, we'll cache size but later on we can use this implementation to always keep a ref to key window for replay

[ioannisj]

Just need to make sure that this will work on all platforms

[ioannisj]

Hey @dfed, would it be possible for you to test your app against this PR when you have a chance? (possibly also verifying that you get the correct $screen_width, $screen_height on your events? 🙏)

[dfed]

PostHogSDK.shared.isFeatureEnabled is already thread-safe, unless your app has to do many operations within that lock, but reading/writing a flag is thread-safe.
I understand your use case, but feels like the deadlock is caused by your own locks/logic, the main thread should be available since most features require main thread access, including our SDK and some of its features.
I agree that we should not contribute to a deadlock hence trying to figure out the best approach here but I'd not hold a lock in the main thread.

You're absolutely right that it's my code, not yours, that is causing the deadlock. However, what I'm doing should be supported by PostHog – PostHog should not be synchronously calling out to a global resource like main. Being able to ensure that PostHog always returns stable data is not possible without a lock or single-threading my calls into PostHog. Holding a lock on main is fine if the work is quick, and this work should be quick.

would it be possible for you to test your app against this #252 when you have a chance? (possibly also verifying that you get the correct $screen_width, $screen_height on your events? 🙏)

done! Looking good to me!