braze-inc / braze-swift-sdk

https://www.braze.com
Other
48 stars 19 forks source link

[Bug]: Why does the validateMainThread run sync on the main thread? #89

Closed azilbershtein closed 8 months ago

azilbershtein commented 8 months ago

Platform

iOS

Platform Version

all

Braze SDK Version

7.1.0

Xcode Version

al

Computer Processor

Intel

Repro Rate

100%

Steps To Reproduce

On BrazeInAppMessageUI.swift there's a validateMainThread function, why does it run synchronize on the main thread in order to send log? theoretically it can create unusual behaviors on the app. Can you change it to async? or not the main thread at all if it's just to send log.

The function: func validateMainThread(for message: Braze.InAppMessage) -> Bool { guard Thread.isMainThread else { DispatchQueue.main.sync { logError(for: message.context, error: .noMainThread) } return false } return true }

Expected Behavior

Should not use the main thread/ sync

Actual Incorrect Behavior

Run sync on the main thread

Verbose Logs

No response

Additional Information

No response

jacksonemiller commented 8 months ago

Hi @azilbershtein, thank you for reaching out. This function does enqueue work on to the main thread. However, as a result of the guard statement, it could only ever block a background thread and not the main thread as it waited for that work to be completed. Is blocking the main thread primarily what you were concerned about? Thanks.

azilbershtein commented 8 months ago

Hi @jacksonemiller Thanks for your answer. The guard statement basically make sure this function run on the main thread, but let's say it doesn't, the background thread will be blocked by DispatchQueue.main.sync, but you don't know what the main is doing at this point, so it can be blocked by another task that might be blocked by the same background task you're running. Basically it's just not good practice to block the main from a background thread :) (at least not for a log). Any chance you can just change it to async instead of sync? (it shouldn't break anything if it's just for logging)

jacksonemiller commented 8 months ago

Ah I think I understand the situation you're worried about now. Makes sense, thanks for pointing this out. I've filed a ticket to implement the change, and it should be released as part of the changes in our next release. Thanks!

jacksonemiller commented 8 months ago

Hi again @azilbershtein,

We just released a fix for this as part of our 7.2.0 release. Thanks!