i have PrinterUtil class that i think can cause memory leak and want to refactor it, but i'm not sure if the new implementation of coroutine is correct
PrintUtil have method printImage that accept listImage, then the listImage get looped and for each loop, it will create new coroutineScope
instead of creating new coroutine inside loop, i think it would be better to create global variable of coroutineScope and call async inside the loop
the old method
Object PrintUtil{
fun printImage(listImage : List<Image>) = CoroutineScope(Dispatchers.Main).launch {
listImage.foreach{
val isSuccess = async{try{
//call suspend method of printing process, throw error if problem occur
}catch(e : Exception){Log.e("error printing",e.message); false}
}.await()
}
}
}
The new method
Object PrintUtil{
val job = Job()
val scope = CoroutineScope(Dispatcher.IO + job)
fun printImage(listImage : List<Image>){
listImage.foreach{
val isSuccess = scope.async{
try{
//call suspend method of printing process, throw error if problem occur
}catche(e : Exception){
Log.e("error printing",e.message)
false
}
}.await()
}
}
//execute on activity onDestroy
fun cancel(){
job.cancel()
}
}
First, let's assume that what you described as suspend method of printing process looks like this:
suspend fun printProcess(image: Image)
Memory leaks in the sense that you accumulate obsolete objects over time that will never get removed from memory can not occur in either of your variants. The worst thing that can happen is that the launched coroutines won't be cancelled and will continue to run until they are finished. Until then they will still occupy memory and all referenced objects will not be cleaned up by the garbage collection. This may be a relevant factor for the Image objects: They will still remain in memory until all coroutines are completed. That also includes all other objects that an Image object may reference. It depends on what Image actually is. Since all of that will get cleaned up eventually, technically speaking this is not a memory leak. It may be undesirable behavior, though.
So cancelling the coroutines is a good way to prevent that. However, using job.cancel() will not only cancel the coroutines, it will also cancel the scope since you created the coroutine scope with that job. That means all further calls to printImage won't do anything since nothing can be executed anymore in that scope. You chose to call cancel in your activity's onDestroy, but should your activity be recreated (what typically happens when just the device configuration was changed, by a rotation f.e.) the PrintUtil object will still be there but it won't work anymore. To fix this you shoud call job.cancelChildren() instead. That will only cancel any coroutines launched in the scope, not the scope itself. Any further calls to printImage will just work as expected.
You can actually also call scope.coroutineContext.cancelChildren() so you don't need the job. That variable and its usage can then be entirely removed.
Cancelling all coroutines like this is only the first step, though. Since cancelling coroutines in Kotlin is cooperative, for this to have any effect printProcess must honor such requests. It can, for example, call yield at appropriate intervals. If it launches any new coroutines they should use ensureActive() (as any coroutine should that wishes to honor cancellation requests).
Having said all that, I think you should reconsider making PrintUtil a global object in the first place. You already identified the dependence on an activity, but as it is you will cancel all coroutine in PrintUtil, not just those initiated from the activity that is getting destroyed. You may only have one activity at the moment so it won't matter much, but it would be better to formalize the dependence on an activity. This way you also cannot forget to call cancel in onDestroy. I would suggest the following:
class ImagePrinter(lifecycleScope: CoroutineScope) {
private val scope = lifecycleScope + Dispatchers.IO
operator fun invoke(listImage: List<Image>) {
listImage.forEach {
// use scope here
}
}
}
Then you can create an object in each activity's onCreate like this:
val printImage = ImagePrinter(lifecycleScope)
lifecycleScope is a CoroutineScope that is automatically cancelled when the activity is destroyed, so there is no need for a cancel method. Make sure you use the gradle dependency androidx.lifecycle:lifecycle-runtime-ktx so you can access lifecycleScope.
You may also have noticed that the function is now declared as operator fun invoke(...). This simplifies the usage of the object. If you want to print an image, just call this:
printImage(listOfImages)
If you want to use it in some other function just provide printImage::invoke as a parameter of type (List<Image>) -> Unit.
Some last words on how you launch new coroutines: Although not very clear, what I think you want to achieve is that printImage should return immediately and successive calls should therefore execute concurrently at the same time. For any given list of images, however, you want this list to be sequentially iterated so the second image is only processed if printProcess of the first image has concluded. Then you should move the loop inside the coroutine:
scope.launch {
listImage.forEach {
ensureActive()
val isSuccess = try {
printProcess(it)
} catch (e: Exception) {
Log.e("error printing", "${e.message}")
false
}
}
}
As @broot already mentioned in the comments, never launch a new coroutine with async and immediately await it. That's pointless, the suspend function can be called directly as seen above.