Skip to content

Suspending version of lazy { ... } #706

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

Closed
clarfonthey opened this issue Oct 15, 2018 · 31 comments
Closed

Suspending version of lazy { ... } #706

clarfonthey opened this issue Oct 15, 2018 · 31 comments

Comments

@clarfonthey
Copy link

clarfonthey commented Oct 15, 2018

Right now, I implement this with:

class LazyValue<T>(private val inner: suspend () -> T) {
    private val latch = Channel(Channel.RENDEZVOUS)
    private val cond = AtomicBoolean()
    private var value = null
    suspend fun get(): T {
        if (this.cond.compareAndSet(false, true)) {
            this.value = this.inner()
            this.latch.close()
        } else {
            this.latch.receiveOrNull()
        }
        return this.value
    }
}

Although this kind of primitive seems reasonable enough to add by default.

@qwwdfsad
Copy link
Member

qwwdfsad commented Oct 15, 2018

You can use val lazyValue = GlobalScope.async(Dispatchers.Unconined, start = LAZY) { inner() }
And even provide your own dispatcher if you need one to offload the computation.

I don't think such shorthand worth its own primitive: we don't have suspend getters and thus can't have by lazy like API.
But let's keep this issue open and see whether we have a demand on suspendable lazy.

@clarfonthey
Copy link
Author

I guess that my main problem is that I'm trying to find a way to confine the evaluation to the scope of the first invocation, which I guess is probably not the way I should be doing this.

I'll think a bit more about this and then report back.

@clarfonthey
Copy link
Author

So, I've been thinking about this a decent amount, and I think that my general conclusion is that it would be nice to have efficient versions of Channel<Unit> and Channel<Nothing>.

I've managed to reduce most of my code to using channels, but I have a few strange cases:

  1. Using Channel<Unit> sort of like a CyclicBarrier, i.e. a leader waits for N signals before it performs an action. In this case, we're not actually waiting for any particular data, just synchronisation. One particular case of this is waiting for N subscribers to a BroadcastChannel before broadcasting anything which seems like something that could be useful to support by default.
  2. Using Channel<Nothing> like a CountDownLatch(1), i.e. a leader closes the channel once it's done performing an action and followers will call receiveOrNull on that channel.
  3. Other synchronisation with AtomicIntegers and AtomicBooleans to ensure exactly one coroutine counts up or down to a particular threshold. I think that this is always going to be there and it's still my opinion that if a synchronisation primitive can be done efficiently with atomics, it should be.

So, I guess that I can create separate issues for 1 and 2 and close this.

@alamothe
Copy link

Wow, lazy blocks a thread while waiting? Are you kidding me?? I thought Kotlin is all about coroutines.

@LouisCAD
Copy link
Contributor

@alamothe

  1. Kotlin 1.0 didn't have coroutines 🤯
  2. lazy is intended for expensive allocation/initialization, to defer it until needed.
  3. As mentioned in the first reply, you can use async and start = LAZY
  4. No, Kotlin is not all about coroutines, you can also write blocking code with Kotlin

@ConnorWGarvey
Copy link

Having this or memoization would be helpful for lazily connecting to services and databases. We have a command line app that sometimes needs to connect to everything and sometimes nothing. Something like by suspend lazy would be great.

@LouisCAD
Copy link
Contributor

LouisCAD commented Apr 5, 2020

@ConnorWGarvey You can already do something similar without delegation:
https://github.com/LouisCAD/Splitties/blob/49e2ee566730aaeb14b4fa9e395a677c3f214dba/modules/coroutines/src/commonMain/kotlin/splitties/coroutines/SuspendLazy.kt#L26

Example usage:

val heavyThing = myScope.suspendLazy { // Or GlobalScope if app global
    val stuff = withContext(Dispatchers.IO) { getStuffFromStorage() }
    initThatHeavyThingWithASuspendFunction(stuff)
}
heavingThing().doStuff()
...
heavingThing().doMoreStuff()

@alamothe
Copy link

2. lazy is intended for expensive allocation/initialization, to defer it until needed.

Precisely, so why would it block a thread? It makes zero sense.

@LouisCAD
Copy link
Contributor

@alamothe Because allocation always blocks a thread, it's actually the CPU looking for space in the memory, and moving stuff if needed.
Making it suspend by default would just make it a little more expensive.

So, it makes complete sense to me that Kotlin stdlib's lazy is the way it is.

If it still doesn't make sense to you, I encourage you to learn more about what happens when you want to allocate memory for some objects, and also, learn how coroutines work under the hood.

And again, I linked an implementation + example of a suspendLazy implementation, feel free to try it if it suits better your use cases. I definitely use it, and it leverages stdlib's own lazy, which I also use alone in some cases.

@fvasco
Copy link
Contributor

fvasco commented Apr 22, 2020

Hi @LouisCAD,
allocation does not block the thread always, you should reconsider the definition of "blocking operation".

Hi @alamothe,
the default lazy implementation uses a lock, so if you use a blocking operation in the lazy block, then that lazy is blocking, that's all.
You can use this suggestion to build an asynchronous implementation.

@LouisCAD
Copy link
Contributor

@fvasco I disagree, allocation is always a blocking operation, and the time the thread requesting the allocation is blocked can vary depending on multiple factors like size requested and state of the memory.

I'm talking low-level here.

Now, I'm tired of this discussion, there's two solutions posted, nothing more to say, so I'm withdrawing.

@fvasco
Copy link
Contributor

fvasco commented Apr 22, 2020

Hi @LouisCAD,
I respect your thoughts, but I fear that the statement "allocation is always a blocking operation" lead to "use Dispatchers.IO for each allocation", and I think it is not the right way.

I'm talking low-level here.

"Thread" and "blocked thread" is an abstraction of Operating System, not a CPU's one, but please correct me if I wrong. Allocation can be -generally- performed by running thread, not by a blocked one.

Thank you.

@LouisCAD
Copy link
Contributor

I get your point now @fvasco and thought about that after writing my message:
Ultimately, every code has a "blocking" part, regardless of what is blocking it, but not all code blocks for long, and not all blocking code should be abstracted away to be supposedly less blocking.

Allocation is indeed performed by the calling thread, so, unless the object hierarchy being allocated is very big and you need the thread quickly (e.g. a UI thread), the most efficient way is to let it be run by the calling thread, not involving any coroutines that'd not improve performance at all.

I think @alamothe has a misunderstanding of the purpose of lazy. Its goal is not to avoid blocking a thread, but to do the computation (an allocation is a short one in a way), only once needed, and share it to future callers.

If the code in the lazy { } lambda takes a significant time to execute, significant enough that other threads are being blocked by the lock and it becomes an issue, then, it probably makes sense to use another strategy, like that solution you @fvasco already linked, or the one I shared before, building on top of it.

@meoyawn
Copy link

meoyawn commented Apr 25, 2020

Defferred<T> is perfectly lazy itself, just use async(start = CoroutineStart.LAZY) { } to compute stuff

@alamothe
Copy link

alamothe commented May 14, 2020

Same as Kotlin doesn't support suspending properties.

It's trivial to implement it on the compiler end, it's very painful for us using the language.

@LouisCAD You're getting too technical for something that's trivial to do. By your logic, suspending functions can't exist either, yet they do.

@LouisCAD
Copy link
Contributor

LouisCAD commented May 14, 2020

@alamothe I don't see evidence that it's "trivial to do". If it really is, then you know better, which means you can submit a KEEP.

By your logic, suspending functions can't exist either, yet they do.

My logic, when stretched by you, but then it's no longer my logic.

Same as Kotlin doesn't support suspending properties.

This is off topic, and there are valid reasons (API design related) to have coroutineContext be the only suspend val to be allowed. Other use cases just need to buy themselves a pair of parentheses.

@alamothe
Copy link

This is off topic, and there are valid reasons (API design related) to have coroutineContext be the only suspend val to be allowed. Other use cases just need to buy themselves a pair of parentheses.

By that logic 🙂 we don't need properties at all. It's just a pair of parens.

@LouisCAD
Copy link
Contributor

@alamothe You are completely ignoring the fact that when you read code, you expect a property to return immediately, while a suspending function is the opposite. But then, again, this is off-topic, so go on Kotlin's Slack if you want to debate that.

@alamothe
Copy link

What does "immediately" mean? Does by lazy return immediately?

How about if it spends 10s doing CPU work vs 0.1s I/O work? Which one is immediate?

This is best left to code owners to decide. It's like saying we will forbid you to name variables with uppercase letters because you deemed that whoever reads the code expects lowercase letters. Not for you to make that judgement.

@alamothe
Copy link

alamothe commented Aug 3, 2020

@LouisCAD we have been using your implementation of suspendLazy to a great success. Thank you sir!

@alamothe
Copy link

alamothe commented Aug 4, 2020

Actually I have a question regarding the implementation. What if it's never called? Looks like it will hang coroutineScope. Is there an easy fix?

	@Test
	fun testSuspendLazy() = runBlocking {
		coroutineScope {
			val l = suspendLazy {
				println("hello")
			}
			// Hangs here
		}
	}

@LouisCAD
Copy link
Contributor

LouisCAD commented Aug 4, 2020

Yes, use GlobalScope or equivalent in this case, or put it in a parent scope that will be cancelled or lives forever.

@alamothe
Copy link

alamothe commented Aug 4, 2020

It's definitely a gotcha. I changed it not to call async until necessary:

private class SuspendLazySuspendingImpl<out T>(
		val coroutineScope: CoroutineScope,
		val context: CoroutineContext,
		val initializer: suspend CoroutineScope.() -> T
) : SuspendLazy<T> {
	private var deferred: Deferred<T>? = null

	override suspend operator fun invoke(): T {
		if (deferred == null) {
			deferred = coroutineScope.async(context, block = initializer)
		}

		return deferred!!.await()
	}
}

Do you see any problems here? (our code is single-threaded)

I think the problem with a scope that lives forever is that they will never get garbage collected i.e. it is a memory leak.

@fikr4n
Copy link

fikr4n commented Feb 16, 2022

My use case is that there's no known/given CoroutineScope, while laziness is not actually needed, but it needs to wait for a coroutine to complete. Which one is better?

  1. Using GlobalScope.
class SomeClass {
    private val someConfig = GlobalScope.async { configLoader.readSomeConfigFromFile() }

    suspend fun doSomething() {
        val c = someConfig.await()
        // ...
    }
}
  1. Using a custom implementation of lazy (see below), no given scope, using the scope of the calling suspending function.
class SomeClass {
    private val someConfig = SuspendingLazy { configLoader.readSomeConfigFromFile() }

    suspend fun doSomething() {
        val c = someConfig.value()
        // ...
    }
}
  1. Using a scope that's ended (cancelled) when the object (SomeClass in the example above) is garbage-collected.
// ...

The custom implementation of lazy

// Please criticize.
class SuspendingLazy<T>(initializer: suspend () -> T) {
    private var initializer: (suspend () -> T)? = initializer
    private var mutex: Mutex? = Mutex()
    private var _value: T? = null

    suspend fun value(): T {
        val m = mutex ?: return _value as T // Warning: unchecked cast.
        m.withLock {
            val i = initializer ?: return _value as T // Warning: unchecked cast.
            val v = i()
            _value = v
            initializer = null
            mutex = null
            return v
        }
    }
}

@kvcache
Copy link

kvcache commented Jul 22, 2022

try this on for size:

/**
 * Use this like a lazy, but because operator getValue cannot
 * be suspend, you'll have to invoke this object instead in a
 * suspend context to receive the value.
 *
 * Use when you want a lazy that is loaded via a suspend fun
 * and you use it in a suspend fun which can tolerate loading
 * the value on a miss.
 */
class LazySuspend<T>(
    private val block: suspend () -> T,
) {
    private val value = AtomicReference<Deferred<T>>()

    suspend operator fun invoke(): T = (
        value.get()
            ?: coroutineScope {
                value.updateAndGet { actual ->
                    actual ?: async { block() }
                }
            }
        ).await()
}

In case usage is unclear:

private val lazyValue = LazySuspend {
  delay(100)
  "hello"
}

private suspend fun usesLazy() {
  val hello = this.lazyValue()
  print(hello)
}

This strategy maps more faithfully to lazy than requiring an explicit context in which to run. It lets you choose whether you use a separate context like dispatchers.io or whatever inside the implementation of your lazy - while defaulting to expecting the simplest use.
Thread safety is obvious through the use of AtomicReference; optimistic double check for low overhead in the usual case. It only enters a coroutineScope if a load is needed or in case of really tight thread races.
I'm not sure any more sophistication is necessary?

@alamothe
Copy link

alamothe commented Feb 17, 2023

@fikr4n Your implementation is definitely appealing, because dealing with coroutine scopes is painful to say the least. The problem is that it is cancellable, which is undesirable for lazy value. Consider:

		val x = suspendLazy {
			println("Calculating x")
			delay(1000)
			10
		}

		try {
			coroutineScope {
				val pAsync = async {
					x()
				}
				val qAsync = async {
					delay(100)
					throw NumberFormatException()
				}
				pAsync.await()
				qAsync.await()
			}
		} catch (e: NumberFormatException) {
			println("Caught")
		}

		println(x())

This will start the calculation two times, because the exception will cancel the first calculation.

EDIT: perhaps this is something that can be fixed simply with withContext(NonCancellable)

@kvcache Your implementation is very problematic, because if the lazy is shared among coroutine scopes where one is canceled due to an exception (unrelated to lazy), then others will be canceled as well. Consider:

		val x = suspendLazy {
			println("Calculating x")
			delay(1000)
			10
		}

		coroutineScope {
			List(10, { it }).map {
				async {
					try {
						coroutineScope {
							val pAsync = async {
								x()
							}
							val qAsync = async {
								delay(100)
								throw NumberFormatException()
							}
							pAsync.await()
							qAsync.await()
						}
					} catch (e: NumberFormatException) {
						println("Caught")
					}
				}
			}.map { it.await() }
		}

It would have been expected here that all exceptions are caught and nothing major happens, however with your implementation JobCancellationException will be propagated outside. So I absolutely wouldn't recommend it.

@fikr4n
Copy link

fikr4n commented Feb 21, 2023

The problem is that it is cancellable, which is undesirable for lazy value. – @alamothe

Yes, in our case it is expected, because the lazy we need is only for optimization: changing "something repeated" become "something cached". Because it is originally repeated, in our case, repeating incomplete operation is not a problem. On the other hand, making it non-cancellable would lead to allowing a never-ending coroutine, furthermore the coroutine scope is unknown.

Thanks

@alamothe
Copy link

@fikr4n Sure! There is also a small bug if initializer returns null:

val x = suspendLazy {
	delay(1000)
	null
}
println(x())
println(x())

The second call will throw NPE because of the bang usage. It is trivial to fix by replacing !! with as T. Other than that, I didn't find any issues with your implementation, and thanks for sharing.

@fikr4n
Copy link

fikr4n commented Feb 23, 2023

The second call will throw NPE because of the bang usage.

@alamothe which bang are you referring to? I think there's no bang there.

@alamothe
Copy link

My bad, I must have copy pasted it wrong and created the bug myself 🤦‍♂️

@midnight-wonderer
Copy link

Kotlin newbie here!

I don't understand why everyone messes with coroutine scopes.
Can we simply delegate to CompletableDeferred?

Something like this:

fun <T> deferredLazy(initializer: suspend () -> T): Deferred<T> {
    val delegated = CompletableDeferred<T>()
    val isStarted = AtomicBoolean(false)
    return object : Deferred<T> by delegated {
        override suspend fun await(): T {
            if (!isStarted.getAndSet(true))
                delegated.complete(initializer.invoke())
            return delegated.await()
        }
    }
}

Warning: not tested, just asked out of curiousity

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

No branches or pull requests

10 participants