sbt / sbt-remote-control

Create and manage sbt process using unicorns and forks
Other
74 stars 14 forks source link

Sync execution queue AND active execution for new clients #168

Closed havocp closed 10 years ago

havocp commented 10 years ago

This PR includes #161 and #162 so merging those first will shrink this one.

This moves eventListeners out of ServerState because we need to atomically modify the listener list and send the sync-up events, which wasn't possible the way the code was structured before.

The PR also has an overhaul of how we use Format, changing it to Writes, sometimes putting an upper bound on writeable types (for e.g. the logSink which now can only accept LogEvent), and actually using the passed-in Writes instead of a hardcoded one.

The main user-visible improvement here is that now when clients connect they are brought up to speed.

jsuereth commented 10 years ago

Again, generally ok, but I have some reservations on the names chosen, especially since it gets REALLY confusing when a bunch of somewhat related subsystems of the project use the term Event, and only one of them gets to "own" the term. I'm going to be pedantic and force that to change before merging. We need to distinguish ExecutionEngine from Logging + custom events.

havocp commented 10 years ago

I think the root problem is https://github.com/sbt/sbt-remote-control/issues/167 that we aren't distinguishing UIContext "custom events" from our protocol.Event / protocol.Message. What we need to do is add TaskEvent(name: String, value: JsValue) (or call it PluginEvent or CustomEvent). Then our UIContext implementation would receive a JsonSink[TaskEvent] instead of a JsonSink[Any]. We could also add a tag trait ExecutionEvent which would just mark "events from the ServerEngine" and then we could give the server engine JsonSink[ExecutionEvent]. Finally we could prohibit non-Message going over the socket at all, and then use JsonSink[Message] rather than JsonSink[Any] inside of ReadOnlyServerEngine. I don't know if this is exactly what you're talking about.

jsuereth commented 10 years ago

@havocp Yeah, I think you're right about CustomEvents needing a wrapper. That's a big flaw.

I also think any stateful messages should be separated from each other, such that the source/owner of these messages is distinct form other Event types. This means I want ExecutionEngine isolated, via a marker trait like ExecutionEvent for events it fires, as well as the object which owns/implies its state.

As for log messages, I only think those are handled differently because of the "buffer startup issues to notify when a cient connects", otherwise they could be treated as all other "stateless" events.

In any case, I think you get what I'd like to see. I think locking the socket to be exactly Message is probably what needs to happen.

havocp commented 10 years ago

I don't think we are quite synced up yet :-) Here is why log events are not stateless and will end up "synchronized" with the execution events: https://github.com/sbt/sbt-remote-control/issues/166 I think logically from the perspective of ServerEngine, the log and execution event sinks can be two different things. But the implementation of those sinks over in ReadOnlyServerEngine will have to connect the two somehow (basically: synchronize on the same lock).

jsuereth commented 10 years ago

Hmm, so you're tying log events to a specific task, which may or may not actually be true. i.e. there are processes happening that could be unrelated to the task engine that are logging and firing log events.

May that's another area where things are somewhat conflated. E.g. "loading" the build doesn't really have an execution ID. How are you assigning its logs?

havocp commented 10 years ago

The taskId in LogEvent is 0 for logs that don't go with a task. I proposed just grouping those temporally in https://github.com/sbt/sbt-remote-control/issues/166 i.e. chunk them up so we have a group of logs between each ExecutionFinished

havocp commented 10 years ago

Rebased on master, other fixes in progress.

jsuereth commented 10 years ago

LGTM, going to check tests.