Add websocket for hot reload #743
Conversation
|
|
||
| // Keep the connection open by sending a message periodically | ||
| Timer("WebsocketHeartbeat", true).schedule(0, 10000) { | ||
| websockets.forEach { it.send(WsMessage("heartbeat")) } |
There was a problem hiding this comment.
This should probably be a ping instead. OkHttp will actually automatically ping from the other side to determine if the connection is alive, but I suppose we don't want to rely on that.
There was a problem hiding this comment.
Looks like OkHttp doesn't do this automatically but there is a setting to turn it on. The connection was closing every 30 seconds previously which is why I added the heartbeat.
I'm not sure if proper ping-pong really matters here. In the current implementation the client websocket will be closed if we don't receive a message every 30 seconds, and the server side should receive a close message most of the time when the websocket closes. There may be a slow resource leak over time where the server keeps some connections open that it shouldn't but given that this is a hot reload server we'll be restarting it constantly anyway and clearing out the old connections. That being said, I'm not particularly opposed to to ping-pong instead of heartbeat, but it's just a bit more code complication.
Thoughts?
There was a problem hiding this comment.
Is that our implementation that gives up after 30 seconds? Or is that just a default timeout?
A ping is nice because it never reaches the user code. Both clients and servers must respond automatically to pings with a pong to conform with the spec. And if it's an automatic OkHttp timeout that automatically closes the connection, the ping will keep it alive automatically.
There was a problem hiding this comment.
Ah, I didn't realize that it was part of the spec, that makes more sense. Apparently http4k does not conform though. It won't respond to OkHttp's pings. Looks like it doesn't properly map frames to the backing Jetty. Womp womp.
I think the 30 seconds may be a server idle timeout, but I don't see an easy way to configure.
So we're left with the heartbeat as the simplest way to make things work, even if it's not quite the proper way of doing things. Definitely less impressed with http4k at this point, though still seems fine for our particular use case. I'll just proceed with this approach unless you have objections.
There was a problem hiding this comment.
Yeah it also doesn't allow sending pings to which OkHttp would automatically reply (the spec is basically symmetric).
Definitely less impressed with http4k at this point
Yep, same.
though still seems fine for our particular use case. I'll just proceed with this approach unless you have objections.
Yep. Merge away.
|
|
||
| companion object { | ||
| const val RELOAD_MESSAGE = "reload" | ||
| val websockets: MutableList<Websocket> = mutableListOf() |
There was a problem hiding this comment.
This needs synchronization for concurrent accesses.
| deploymentRegistry.start( | ||
| deploymentId, | ||
| DeploymentRegistry.ChangeBehavior.BLOCK, | ||
| ZiplineServerDeploymentHandle::class.java, | ||
| server | ||
| ) | ||
|
|
||
| // Keep the connection open by sending a message periodically | ||
| Timer("WebsocketHeartbeat", true).schedule(0, 10000) { |
There was a problem hiding this comment.
This is terse, but the right way to do this since ~2004 is ScheduledExecutorService. Or perhaps something with a coroutine.
| Pair("zipline", ContentType.TEXT_PLAIN) | ||
| ) | ||
| ) | ||
| val server = PolyHandler(http, ws).asServer(Jetty(8080)) |
There was a problem hiding this comment.
Switching from SunHttp to Jetty is a bummer. You probably lose a decent amount of start-up time here.
MAYBE we finish OkHttp 5.x someday and we can use MockWebServer here, which is fast, small, and websockets-aware.
There was a problem hiding this comment.
Yeah, I would love to use MockWebServer, then we could get good testing too. What is the dependency on OkHttp 5? Can't we add proper WS support in 4.x?
Switches from Sun to Jetty server to add websocket serve capabilities. When a Kotlin file is changed a build will be triggered, executing this task. If the server hasn't been started, we will start it. If the server is up already, then we will instead send a websocket message to all connections letting them know that an update is available and they should reload the manifest.
Use of the websocket connection is entirely optional and will just provide a slightly faster hot reload experience. Callers can still poll the manifest (and should fall back to this if the websocket connection fails).