Redesign file watching and add client listeners #345
Conversation
It is important that we propagate these variables from the use site even if the state is cached because they may be different for different clients. The common options are also crucial (because we want to enable future modifications of `System.in`, `System.out`, etc).
This folder is meant to be used to test bloop binaries in development.
The previous file watching logic was a monstruosity because: 1. It relied on `Await.result` to get the state for every of the actions run on file events. 2. It was difficult to reason about. 3. It was not possible to cancel it. This new implementation uses the nifty `Observer` and `Observable` interfaces to implement file watching in a reliable way, closing the watcher when the implementation is cancelled or whenever an exception happens in the watcher. This issue fixes #257, which was caused because file watchers of previous clients (connections) were still running in the background and acting on events.
The previous one didn't work because `Observable` returns a list of tasks, and that task cannot be processed and flattened until the stream of file events has finished (never). This is bad, it means we cannot fold left values produced by observables and then pass them in to functions that return `Task`s. For that, I've discovered you need `Consumer`, and there is one awesome consumer that does exactly what we want: the one created with `Consumer.foldLeftAsync`.
Fixes #257 which was caused because the cancellation of tasks for file watching did not work correctly when `CTRL-C` was used.
| import java.util.concurrent.atomic.AtomicInteger | ||
|
|
||
| import scala.Console.{CYAN, GREEN, RED, RESET, YELLOW} | ||
|
|
||
| import bloop.io.AbsolutePath | ||
| import com.martiansoftware.nailgun.NGCommunicator |
| case object SocketError extends CloseEvent | ||
| case object SocketTimeout extends CloseEvent | ||
| case object SessionShutdown extends CloseEvent | ||
| case object InternalError extends CloseEvent |
There was a problem hiding this comment.
Why not using NGClientDisconnectReason directly?
There was a problem hiding this comment.
Because I want not to depend on nailgun data structures as much as possible. Who knows what we'll use in the future. It didn't take a long time to do this, but in my opinion it's worth it 😄
| pool.addListener { | ||
| case e: CloseEvent => | ||
| if (!handle.isCompleted) { | ||
| System.out.println( |
There was a problem hiding this comment.
So that's writing to stdout of the server? Isn't that what ngout is for?
There was a problem hiding this comment.
Indeed, but in this case the stdout that we see here is already ngout! I'll change it though, for clarity 😄
|
@Duhemm It was hard, but got it working. Now our logger also contains information about the server (which will be quite handy in the future!). |
| run(mainTest(args)) | ||
| } | ||
|
|
||
| private[bloop] def mainTest(args: Array[String]): NGServer = { |
There was a problem hiding this comment.
Not sure that's the best name? getServer? instantiateServer?
|
|
||
| // We check here the logs because until 'exit' the server doesn't flush everything | ||
| val serverInfos = rlogger.getMessages().filter(_._1 == "server-info").map(_._2) | ||
| val cancellingTasks = serverInfos.filter(_.contains("Cancelling tasks...")) |
There was a problem hiding this comment.
serverInfos.count(_.contains("Cancelling tasks...")) ?
|
Done! Please check your feedback is properly addressed 😄 |
|
Great! Please merge when it's green 😄 |
jvican
force-pushed
the
topic/cancel-gracefully
branch
from
df6f3ea
to
4921792
Compare
Nailgun has this great concept of client listeners: closures that are called
whenever the nailgun server thinks that the client has disconnected. These
closures receive the kind of event that the nailgun server got, and executes
some logic.
This concept allows bloop to register callbacks that are run to clean
resources. Cleaning resources is paramount to keep the right behaviour of
long-running tasks like file watching and bsp servers.
This PR does three things:
ClientPool.consumers that help us reason about file watching events more reasonably.
Fixes #257 which was caused
because the cancellation of tasks for file watching did not work correctly
when
CTRL-Cwas used.