Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure thread safety #32

Closed
wants to merge 7 commits into from
Closed

Ensure thread safety #32

wants to merge 7 commits into from

Conversation

svara
Copy link
Collaborator

@svara svara commented Feb 28, 2024

This PR ensures all classes and functions are thread-safe by adopting the @MainActor attribute.

With the adoption of async/await we've introduces potential race conditions and made some of the classes not thread safe.
A MainActor is a globally unique actor who performs his tasks on the main thread.

I think adopting MainActor makes sense because:

  • it ensures thread safety throughout the library.
  • BridgeDelegate is usually instantiated in a UIViewController, which conforms to MainActor itself, thus no additional code changes are needed.
  • BridgeComponents are usually used to interact with UI, which must run on the main thread.

This PR also fixes an error when using the async version of evaluateJavaScript. More info here.

Source/Bridge.swift Outdated Show resolved Hide resolved
Copy link
Member

@joemasilotti joemasilotti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love that we can move @MainActor to the class level, great call! I added one question and one request, otherwise this looks good to me.

Source/Bridge.swift Outdated Show resolved Hide resolved
Source/Bridge.swift Outdated Show resolved Hide resolved
Tests/BridgeComponentTests.swift Outdated Show resolved Hide resolved
Comment on lines 87 to 89
do {
return try await webView.evaluateJavaScript(javaScript)
return try await webView.evaluateJavaScriptAsync(javaScript)
} catch {
Copy link
Collaborator Author

@svara svara Feb 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When using async/await version of evaluateJavaScript, WKWebView expects JavaScript to return with a value (something other than Void). If there's no value returning from the JavaScript that you evaluate it will crash because it tries to unwrap a nil value to Any.

@svara
Copy link
Collaborator Author

svara commented Feb 28, 2024

I've tested these changes in the Calendar app, and so far haven't seen any regressions or issues.

@svara svara requested a review from jayohms February 28, 2024 19:32
@svara
Copy link
Collaborator Author

svara commented Feb 28, 2024

@joemasilotti Unfortunately I'm running into some issues. Will continue tomorrow.

@svara
Copy link
Collaborator Author

svara commented Feb 29, 2024

@joemasilotti Could you please give this branch a try in one of your projects? I'm running into an issue and can't seem to get it to work anymore. The components are successfully registered when the bridge sends 'ready', but after that, no more messages are received from the bridge. I've double-checked the code, but I'm unable to identify what might be causing this. I'm running out of ideas.

@zoejessica Could you please double check the code?

@joemasilotti
Copy link
Member

I'm not seeing any components but I do see bridgeNotInitializedForWebView in the console.

I think making Bridge.initialize() async is breaking it. I need to return the web view right away to work with Turbo Navigator but it isn't initialized until the completion block calls.

Turbo.config.makeCustomWebView = { configuration in
    configuration.defaultWebpagePreferences?.preferredContentMode = .mobile

    let webView = WKWebView(frame: .zero, configuration: configuration)
    webView.makeInspectableInDebugBuilds()
    Bridge.initialize(webView) { }
    return webView
}

If this change is required we can refactor the makeCustomWebView block to be async as well.

@svara
Copy link
Collaborator Author

svara commented Feb 29, 2024

@joemasilotti I've seen bridgeNotInitializedForWebView when the web view is required right away. Subsequent calls do work though, however the components are still broken.

Man, it should be trivial to start a Task on the main actor in a synchronous way, when the task is started from the main thread. But nope. I'm still looking for alternatives.

@joemasilotti
Copy link
Member

Man, it should be trivial to start a Task on the main actor in a synchronous way, when the task is started from the main thread.

Agreed, I just want a blocking call!

Does initializing the Bridge have to be asynchronous? What if there was a non-async option, too?

@svara
Copy link
Collaborator Author

svara commented Feb 29, 2024

Does initializing the Bridge have to be asynchronous? What if there was a non-async option, too?

Having @MainActor attribute enforces it. However, I can remove it from the class level, and only mark the async function with it.

@joemasilotti
Copy link
Member

joemasilotti commented Mar 1, 2024

Excellent, everything is working for me, too!

@joemasilotti
Copy link
Member

I had to manually merge this on the command line for some reason. Thanks @svara!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants