search

sbt / sbt-remote-control

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

Changes to watch() behavior #97

Closed havocp closed 10 years ago

havocp commented 10 years ago

Writing an integration test I tried to use the SbtClient.watch() API and I think there are some problems to solve. Haven't worked out all solutions yet. Just opening this issue as a place to discuss, I can work on a patch next week.

  1. watch(task) runs that task immediately. I think it's very plausible that a client wants to know if a task's value changes (and might install a bunch of watchers on startup for that reason), but doesn't want to immediately run the task. My integration test wants to do this for example; but I think it's also pretty plausible for Activator or an IDE.
  2. EXCEPT if there's already another listener - then ListenerManager does not send the ListenToValue and therefore there is not an immediate/guaranteed value notification. So multiple listeners using the same client do not know whether they will get an initial value.... or if you have working code that relies on getting the initial value, then you add a second listener... maybe your code breaks. Side effect.
  3. I don't see a QuitListeningToValue - if watchers goes to zero we still keep getting the value change events, but ignoring them, I believe. Same for the other kinds of listening, not only value watchers, I guess.

None of these should be a big deal to fix, I think for the first two we'd maybe add a flag for "give me an immediate notification" which would then always or never give an immediate notification - or alternatively, we could never give immediate notification and provide a separate method/request which would force a notification. Not sure exactly. Anyone with an opinion let me know...

jsuereth commented 10 years ago

Yeah, so there's a big missing piece here:

  1. We don't really track the previous values of tasks yet. We just always report values no matter what. So, first we need that cache of previous value
  2. We need a way to diff the values (maybe == is ok)?, and then we send the notification if the cache doesn't match the currently cached value
  3. When someone registers to listen we do the following:
    • If there is already a cached value we send it to them
    • If there is no cached value, we schedule the task to run so we can cache the value
  4. We should add a "quit listening" that also clears the cache of values
havocp commented 10 years ago

I was going to suggest that caching the values is involved in the solution but we have talked about a separate layer around SbtClient which does caching/observables right?

I think I can add "Unlisten" requests quickly with no structural changes so I might do that first.

I do think we need a way to watch without scheduling the task. I think Activator will want to know latest results whenever they exist, but we don't always want to launch a compile and test on startup. My suggestion here is to just separate watching and "notify current value" on the protocol level (two separate requests) and then manage it on the client API level (client has some API/logic to decide when to ask for current value notification). I can make an initial patch that keeps current behavior but with two requests and then we can start evolving the client.

Right now for me this is just a big yak shave to make it easier to finish my "execution requests and events" integration test....

havocp commented 10 years ago

I think this is all fixed, forgot to close it.