Make a CodeHost state machine #1667
Conversation
|
I’d like to add tests before reviewing this. |
| import kotlinx.coroutines.SupervisorJob | ||
| import kotlinx.coroutines.cancel | ||
| import kotlinx.coroutines.job | ||
| import kotlinx.coroutines.launch | ||
| import kotlinx.serialization.json.Json | ||
|
|
||
| internal class FakeCodeSession( |
There was a problem hiding this comment.
I think I can refactor the ZiplineCodeSession state management to share a lot more code with this fake.
| override fun cancel() { | ||
| if (canceled) return |
There was a problem hiding this comment.
I don’t like that these are being canceled multiple times. It’s not a bug, but it’s inelegant.
| fun setUp() { | ||
| runBlocking { | ||
| codeHost.start() | ||
| eventLog.takeEvent("codeHost.collectCodeUpdates()") |
There was a problem hiding this comment.
I added an event for this so I can tell when we’re receiving new code and when we’re not. In this test it’s not that interesting!
| content.unbind() | ||
| } | ||
|
|
||
| /** CodeHost doesn't have to stay resident forever. */ |
There was a problem hiding this comment.
This is probably the most exciting thing about the PR. We can now shut down a Zipline completely and start it up again. That’ll be potentially great for saving memory.
swankjesse
force-pushed
the
jwilson.1101.scope_to_code_session
branch
from
452ef54 to
87b9818
Compare
swankjesse
force-pushed
the
jwilson.1102.code_host
branch
from
79b46c1 to
bbdeb4d
Compare
This simplifies much of the internal complexity of TreehouseApp. We can also delete FakeCodeHost because the single implementation is suitable for unit testing. Closes: #1660
Also adopt CoroutineScope more aggressively in CodeSession for lifecycle management.
swankjesse
force-pushed
the
jwilson.1102.code_host
branch
from
bbdeb4d to
64dc227
Compare
| @@ -62,7 +71,7 @@ public class TreehouseApp<A : AppService> private constructor( | |||
| * instance may be replaced if new code is loaded. | |||
| */ | |||
| public val zipline: Zipline? | |||
| get() = codeHost.session?.zipline | |||
| get() = (codeHost.codeSession as? ZiplineCodeSession)?.zipline | |||
There was a problem hiding this comment.
Is codeSession not being a ZiplineCodeSession valid?
If not, perhaps we can make CodeSession itself a type parameter?
There was a problem hiding this comment.
Lemme explore that in follow up!
| class Running<A : AppService>( | ||
| override val codeUpdatesScope: CoroutineScope, | ||
| override val codeSession: CodeSession<A>, | ||
| ) : State<A>() | ||
|
|
||
| class Starting<A : AppService>( | ||
| override val codeUpdatesScope: CoroutineScope, | ||
| ) : State<A>() | ||
|
|
||
| class Crashed<A : AppService>( | ||
| override val codeUpdatesScope: CoroutineScope, | ||
| ) : State<A>() | ||
| } |
There was a problem hiding this comment.
Personally, I would've loved to see the states hoisted inside the respective states, in order to get the true benefits of making a state machine (i.e., it being easy to follow how one state gets to the next, a thing which is still difficult in this PR).
Apologies if that was a planned follow up!
There was a problem hiding this comment.
Yeah lemme explore that in follow up.
There was a problem hiding this comment.
A month later . . . we looked into this, and didn’t find something I loved!
@veyndan if you’d like to run with this, please do!
This simplifies much of the internal complexity of TreehouseApp. We can also delete FakeCodeHost because the single implementation is suitable for unit testing.
Closes: #1660