refactor: simplify websocket code #425
Conversation
|
Does it work if you put your laptop to standby/sleep and resume later? |
|
@sockmaster27 Some conflicts, unfortunately :) |
|
I added some concerns + I am not sure if all the old code has been removed. |
|
For whatever reason, each reconnect spawns a new process, and I can't figure out why. It doesn't seem to our code that runs. It doesn't impact anything though, so I don't think it's super important since it behaves how it's supposed to. |
A new Flix compiler instance? Those are pretty heavy. We dont accidentially send an exit? https://github.com/flix/flix/blob/master/main/src/ca/uwaterloo/flix/api/lsp/LanguageServer.scala#L255 |
|
Found it. If you're curious, it was reinitializing and spawning a new REPL on every reconnection. |
server/src/engine/socket.ts
Outdated
| setTimeout(onOpen, 0) | ||
| } | ||
|
|
||
| webSocket.removeEventListener('open', openHandler) |
There was a problem hiding this comment.
Can you explain some of the above code, I am not sure how it works.
There was a problem hiding this comment.
I've tried improving it, but let me know if there's still something that should be explained better.
|
Did you try that this works in practice? E.g. if you start VSCode and enter sleep or hibernation then it works when you resume? |
It does, but I've only managed to make it lose connection a single time. Anyway, the tests should cover it. |
|
I think there is confusion: I don't want to restart the compiler. The connection will drop, but we should just be able to reconnect. |
|
Let me sketch the architecture I have in mind: When the extension starts, before the compiler is started, we find a free port. We then start the compiler. This allows us to run as many VSCode instances and Flix compilers as we want. Each gets a new fresh port that they can communicate over. The Flix compiler is never restarted. The only thing that can happen is that during standby the connection between VSCode and the compiler may drop. However, simply reconnecting in loop should make everything work again. Does it make sense? |
|
But sometimes the compiler process dies, e.g. if you (accidentally) kill it in task manager. Right now this will cause the compiler to be relaunched, otherwise VS Code would sit in a loop trying to reconnect until you were to manually restart it. The restarting will only happen if the compiler is dead, which the extension can infer by seeing if the socket has been freed to the system. Otherwise it will simply reconnect to the same instance. |
It should really not. The problem with restarting the compiler is resource leaks. I don't want that. |
|
Resource leaks in what way? |
You assume that because the port has been released the process has died. |
Yes. This is what was done before, and I have yet to see a case where it wasn't true. Admittedly, this is an edge case, but so is losing connection to the server, so I'm thinking that we might as well handle it correctly while we're at it. |
|
I don't want this effect:
I want the compiler to be a server -- started once -- that you can reconnect to. |
|
Again, I really don't see that as a risk, and I think relaunching is the correct thing to do, but if that's what's blocking I'll remove the feature. |
|
Can you double check:
|
Yes, everything works on my machine. If we want to be extra sure, a non-Windows user might also want to try it. |
Outsources the reconnecting part to reconnecting-websocket, like it's done on play.flix.dev.
Fixes #305