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

fix (macos) APP crash when a web custom protocol request is aborted before or during response #1214

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

thomastschurtschenthaler

closes 1189

I am new to rust and object-c, so not sure if everything is alright with the memory management, but it works well for my tests so far.
Testfile attached - every third request is aborted.
main.rs.zip

@thomastschurtschenthaler thomastschurtschenthaler requested a review from a team as a code owner April 5, 2024 16:22
@FabianLars
Copy link
Member

Thanks for contributing! I normally keep quiet about objc heavy macos issues/prs but since i spent the whole day on the startTask code yesterday i do have some opinions this time.

I personally would like to reduce our reliance on ivars where possible, even if that means having to use global variables (via once_cell::Lazy for example). To make this easier, i saw that others stored just the hash of the object, for example like here. I did not hear anything bad about this approach in this context so i assume it's safe enough.

Also, i wonder if we have to check this multiple times and not only once before it actually starts sending it. For that to make sense we'd probably have to chunk the didReceive call so maybe that's overkill 🤔I don't know how fast the didReceive call is, and how it behaves if stopTask is triggered just before we call didReceive (but after the check you added) 🤔

@thomastschurtschenthaler
Copy link
Author

I made changes to use the tasks hash value in the store as you suggested, also the lock ist now released only after the call to task->didFinish.
still works fine in my tests.
Though I wonder the swift code you linked doesn't use any lock/sync for the dictionary at all..

@morajabi
Copy link
Contributor

morajabi commented Apr 8, 2024

I don't know what I'm doing wrong but as soon as I run tauri with this change it crashes.

@thomastschurtschenthaler
Copy link
Author

I managed to solve the sync/lock problem by sending the task response on the main thread (via NSObject.performSelectorOnMainThread).

@FabianLars
Copy link
Member

Won't this cause issues with large payloads?

@thomastschurtschenthaler
Copy link
Author

It should not as the response data payload is already provided as byte array by the custom thread.
Calls to the task like didReceiveResponse and didReceiveData seem to run on the main thread anyways - thus my issues with deadlocks when I locked the main thread in the stop_task call.
I did some tests with large data (3 parallel requests with about 3.5GB response payload) and the performance seems not to be affected.
For streaming the Responder API would probably need to provide some way to send multiple smaller chunks of data - and then call didReceiveData for each chunk.

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