skolson / KmpIO

Kotlin multi-platform simple File I/O library
Other
32 stars 2 forks source link

iOS crash #9

Open jbruchanov opened 1 month ago

jbruchanov commented 1 month ago

There seems to be some invalid memory management/memleak...

Following code is crashing like 2seconds after execution It correctly creates "empty" zip file, but it's just crashing little bit after (simulator or real device)

launch(Dispatchers.IO) {
    //"${NSTemporaryDirectory()}/test.zip"
    val zip = platformInfo.storageInfo.tempFolder().subPath("test.zip")
    val zipFile = ZipFile(File(zip.uri), FileMode.Write)
    zipFile.close()
    println("zip:$zip, len:${zip.length()}")
    //zip:IosPath(uri='/Users/scurab/Library/Developer/CoreSimulator/Devices/61B1BAD5-33D9-44CE-A306-04DF5CEB04F8/data/Containers/Data/Application/4AF271F4-0A33-44C6-B1BD-F037839D00F7/tmp/test.zip'), len:22
}

error:

App(29562,0x16dab7000) malloc: *** error for object 0x115ab03f0: pointer being freed was not allocated
App(29562,0x16dab7000) malloc: *** set a breakpoint in malloc_error_break to debug
skolson commented 1 month ago

Hmm, this looks like an attempt to double-free some pointer, but everything in the library is memscoped, so it should be tough for this to happen. I created a unit test specific to iOS:

    @OptIn(ExperimentalNativeApi::class)
    @Test
    fun zipFileEmpty() {
        runTest {
            NSFileManager.defaultManager.temporaryDirectory.path?.let {
                val zipPath = "$it/test.zip"
                val zipFile = ZipFile(File(zipPath), FileMode.Write)
                assertEquals(0, zipFile.entries.size)
                zipFile.close()
                println("zip: $zipPath")
            } ?: run {
                fail("no temp directory found")
            }
        }

    }

This test ran fine in the simulator, resulting in this line on the console: zip: /var/folders/1v/wv70896s5tz7htjmh1xp61pw0000gn/T/com.apple.CoreSimulator.SimDevice.D18C0960-7FD3-4339-81B9-5A8FC50E4CF7.Standalone.BB7C9CFB-04A3-45AB-A70E-0C5BA694BE56/test.zip I can add some stuff to the test but the basic use case of creating an empty zip file works. I am unsure what would be next best step without a re-producible problem.

skolson commented 1 month ago

I'll push the new test to the repo in case it is useful to you. Also fyi the test above was on simulator 17.4.

jbruchanov commented 1 month ago

@skolson

I was able to pinpoint it little bit better

val destinationPath = NSFileManager.defaultManager.temporaryDirectory.path!!
val zip = File("$destinationPath/test.zip")
println("zip:$zip exists:${zip.exists}")
//(zip:/Users/scurab/Library/Developer/CoreSimulator/Devices/61B1BAD5-33D9-44CE-A306-04DF5CEB04F8/data/Containers/Data/Application/492E2BDD-1056-4494-952E-4764082A04C3/tmp/test.zip exists:true)
zip.delete()
val zipFile = ZipFile(zip, FileMode.Write)
zipFile.close()
println(zip.path)

that zip.delete() seems to be problematic... (probably forgot to include it in initial code due to a lot of different testing) if I comment it out, it doesn't crash (doesn't matter if file exists or not) btw it doesn't crash in the test, it must be real app device/simulator...

Also one point, is there any particular reason to have everything suspendable ? After very quick look I didn't actually see any suspendable/nonblocking code, so it's imo pointless to bring whole coroutine machinery if there is no use of it...

skolson commented 4 weeks ago

The library uses suspend because of the pervasive disk IO, which is typically not desired on the main thread of any of the platforms with a UI. I do the same with database operations in another library.

So the delete function is really simple (see the code). I'm suspecting there is some issue with the use of throwError as it creates an NSException pointer. The test below runs fine in the simulator, but there's no Swift or ObjC code dealing with the NSException pointer it would make in the test - it's all Kotlin. Also in the test the delete doesn't fail, so throwError should do nothing. If there's a chance in your iOS implementation an exception is being thrown on a delete fail, and higher level non-kotlin code is catching it?

New test that still works in simulator:

    fun zipFileEmpty() {
        runTest {
            NSFileManager.defaultManager.temporaryDirectory.path?.let {
                val zipPath = "$it/test.zip"
                val fil = File(zipPath)
                assertFalse(fil.exists)
                assertFalse(fil.delete())
                val zipFile = ZipFile(File(zipPath), FileMode.Write)
                assertEquals(0, zipFile.entries.size)
                zipFile.close()
                assertTrue(fil.exists)
                assertTrue(fil.delete())
                assertFalse(fil.exists)
                println("zip: $zipPath")
            } ?: run {
                fail("no temp directory found")
            }
        }
    }
skolson commented 4 weeks ago

One easy option would be to change throwError to a Kotlin exception and see if it acts better. The library uses throwError in a number of places in the Apple common code. If throwError in IOS turns out to be problematic I'd switch them all to Kotlin exceptions.

jbruchanov commented 4 weeks ago

@skolson for the crash issue: I can't really tell, given the fact that crash is happening delayed, it might be very vell bug on KMP side. In my case I just call the ^^ code from button click handler, so 99% sure it's not my code causing it..

for the suspend functions: making function suspendable doesn't make any guarantees about thread execution it's imo misuse of coroutines. Having blocking code inside, sort of kills whole idea about coroutines being non blocking primitives. also it creates some obstacles in a code, you might be sometimes limited because you need to drag "coroutines" context all over the place which might be annoying if it's unnecessary... anyway, it's your library, just pointing out that whole coroutines stuff looks unnecessary, because the library code is mostlikely blocking only/mostly...

also another point. Looking at the NSErrorException I'm not totally sure about the purpose, surely if there is an exception you want to be able to catch in common, but having that defined in appleMain, assuming that's gonna be inaccessible from common => impossible to catch it directly as you don't have access to NSErrorException type there...

skolson commented 3 weeks ago

We'll have to agree to disagree on the suspend issue. It's out of scope for this issue anyway.

Here's the code for the delete function, in case you hadn't already looked at it.

    open suspend fun delete(): Boolean {
        return if (exists) {
            throwError {
                fm.removeItemAtPath(path, it)
            }
        } else
            false
    }

throwError is Kotlin's standard way of dealing with an NSException from the NSFileManager operation. I'm unsure what you think should change. The intent was for the NSException, which is apple-specific, to be caught by apple code. There is no intent in the library to wrap all/any apple exceptions in Kotlin MP code (write my own throwError) so they can be handled in common code.

There is no other memory management code in this function. Without a reproducible test case, I'm unsure what to try next to help. If a Kotlin bug with throwError processing is suspected, then they will definitely want a reproducible test case to support a Kotlin bug/issue report.

jbruchanov commented 6 days ago

Hi, sorry for very late answer. Here is a reproducer. https://github.com/jbruchanov/KMPMultiProject/tree/KmpIO_9

Open the LibsProject, just run the app on iOS. Click on "ZIP" button multiple times with delay in between like 1s... usually 3 - 4 cliks and then wait like upto 10s => 💥

rohitst commented 2 days ago

I can reproduce this as well, with just this:

val zipName = CommonStorage.tempZipUploadName
val zip = com.oldguy.common.io.File(platformStorageUtils.getInternalStorageDir().getPath() + "/",
            zipName
        )
val zipFile = ZipFile(zip, FileMode.Write)
        zipFile.open()
        zipFile.close()

When the GC runs it crashes, usually in like 5-6 seconds. iOS only crash, Android is fine.

skolson commented 2 days ago

Thanks to both of you this will undoubtedly be a big help. I will start working this tomorrow.

On Fri, Apr 26, 2024, 8:54 AM Rohit Talati @.***> wrote:

I can reproduce this as well, with just this:

val zipName = CommonStorage.tempZipUploadName val zip = com.oldguy.common.io.File(platformStorageUtils.getInternalStorageDir().getPath() + "/", zipName ) val zipFile = ZipFile(zip, FileMode.Write) zipFile.open() zipFile.close()

When the GC runs it crashes, usually in like 5-6 seconds. iOS only crash, Android is fine.

— Reply to this email directly, view it on GitHub https://github.com/skolson/KmpIO/issues/9#issuecomment-2079661257, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEXDDTBAUJZEEFP5YXW7KGTY7J2DBAVCNFSM6AAAAABFD7JBPOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANZZGY3DCMRVG4 . You are receiving this because you were mentioned.Message ID: @.***>