Kulla API changes: shifting to events

[Brian Goetz]

I'm looking at the API as it now stands; I realize there are still 
probably some changes in the pipe, so consider this a "checkpoint" review.

The event structure you outline in the message below seems reasonable to 
me.

Some additional Qs and comments:

1.  I see you left SourceCodeAnalysis as a separate, but dependent, 
abstraction.  I can imagine several reasons:

  - Just not finished refactoring yet, to pull up SCA methods into JShell;
  - You wanted to leave them on SCA, because that sent a valuable 
"grouping" method to the reader of the code;
  - You have decided that SCA is an optional part of the API (if so, 
spec should say), so you want to leave room for sourceCodeAnalysis() to 
return null / throw UOE, and having one branch-point for this is better 
than N.

Related style nit: in SourceCodeAnalysis, the 'static' on nested enums 
and the 'public' on nested classes of interfaces are redundant.  Also, 
the triple-nesting of SCA.CompletionInfo.Completeness seems excessive; 
I'd pull Completness up into SCA.

2.  I see you return a List<Event> from eval.  Is this where you landed, 
or a waypoint?  Am I to assume that the List<Event> returned from 
evaluation is a convenience copy of events also delivered to listeners?

If so, this seems a pragmatic compromise; a client that goes whole-hog 
on events can ignore the returned list, and a client who really just 
wants to know "what happened when I eval'ed" can process the lists and 
ignore the events.  And a client that wants both can even use the 
identity of events to track "I already processed this guy", and avoid 
duplicate-processing, say with a WeakHashSet.  (Right?)

3.  I would rename 'stop' to 'tryCancel()'.  You've already got a 
close() method (courtesy of AutoCloseable); the value-add of `stop()` is 
that it TRIES to CANCEL any outstanding evaluation.  Which means that 
the correct shutdown idiom would be:

    repl.tryCancel()
    repl.close()

However, cancellation is a tricky beast (See JCiP Ch7.)  I would 
consider returning either:
  - boolean indicating that the cancellation succeeded or failed
  - a Future on which the client can wait (perhaps with a timed wait) 
for cancelation to complete (if its possible to detect when it does.)

I believe the latter is practical if you wrap the eval() implementation 
with a semaphore.  In which case the shutdown protocol becomes:

     Future f = repl.tryCancel();
     f.timedWait(reasonableAmountOfTime);
     repl.close();

Also, you've explicitly introduced concurrency into the API, so you just 
bought yourself a call option on a bag of data race bugs...so let's put 
that on the code-review list.

Now that you have ONE method that is explicitly safe to call from other 
threads, you have to raise your spec game about what operations are or 
are not safe to call from multiple threads.  Lazy approach: declare 
that, with the exception of the tryCancel method, the JShell API is 
*not* designed to be thread-safe, and that the caller should arrange for 
all method invocations (including invocations in response to event 
delivery, whose thread context are not guaranteed) should be done in a 
fixed thread.  (We can talk about ways to enforce this if you want to go 
even further with this.)  Less lazy, but harder, approach: try to make 
some subset of JShell thread-safe.

Can close() be called from another thread?  If so, does it implicitly 
call tryCancel()?  If not, spec of close() should guide reader towards 
considering tryCancel in this case.

4.  I don't see any way to *remove* things from the class path.  Nor a 
way to inspect the contents of the class path.

5.  Do you wrap all returned lists/maps with unmodifiable?

6.  Doc for subKind leaves me wondering what subKind is and why I should 
be interested.

7.  Naming convention: for the key-accepting methods, consider 
xxxFor(key)?  diagnosticsFor(key), signatureFor(key), etc.

8.  onStateShutdown -> onShutdown.

9.  Should JShell be an interface or an abstract class?  Both have pros 
and cons.  Interface is cleaner, but has a higher spec burden.  Abstract 
class ties you into being the only implementor of JShell, but allows you 
more control.

10.  I don't think onShutdown needs to be passed the JShell instance. 
(1) Simple clients have only one JShell instance, and (2) more complex 
clients can capture the JShell instance:

     jShell.onShutdown(() -> {
         shellRegistry.remove(jShell); <-- ordinary lambda capture
     });

So I would have this method just take Runnable.

Spec for all onXxx methods must say that the caller should not count on 
the event being delivered in any specific thread context, and if the 
handler intends to invoke any jShell methods, it is the handlers job to 
enqueue them for processing on the thread to which JShell is confined.

11.  EvalException has a Javadoc ref to nonexistent method Class.asName.

12.  Key.beforeStatus -> previousStatus.

For Key.causeKey, you should distinguish between "proximate cause" and 
"ultimate cause", and make it clear which you mean.  For example:

repl> void a() { b(); }  // no b yet, so a is not yet valid
repl> void b() { c(); }  // no c yet, a and b not yet valid
repl> void c() { } // yay, everyone valid!

When you get a transition event for a, is the cause key bKey or cKey?  I 
would *think* that the answer is cKey for two reasons:

1.  Seems more useful, to associate transitions with the root cause;
2.  It avoids ambiguities, as in this case:

repl> void a() { b(); c(); }
repl> void b() { d(); }
repl> void c() { d(); }
repl> void d() { }

The "proximate cause" strategy would cause the causeKey for validating a 
to randomly be bKey or cKey, but the "root cause" strategy would make it 
stably dKey, which seems preferable.  We should (a) pick one, (b) pick 
the right one, and (c) spec which one we picked.

13.  Feels that it would be more consistent with the rest of the API for 
SubKind to be a nested type of Key, sibling to Key.Kind.

On 6/10/2015 2:10 PM, Robert Field wrote:
> As I'm working on implementing this, I've run into a place where the
> prettiness of the API makes for big pain for the API consumer -- in my
> case the jshell tool.  The Result structure, tentacled as it is, tied
> together information which needs to be processed together. Having
> separate key status events, value events, and exception events would
> require collecting all the events and cross linking them to get the kind
> of information  we can present now.  It is also kind of a lie, one only
> sees values through events at the point of status changes,
>
> Value and exception data only occur paired to status change points. If,
> instead of being separate events, the value/exception information is
> included in the event now named KeyStatusEvent then the information
> would be tied nicely -- even though this means the event is a bit
> bulky.  So, with this, the fields of the event would be:
>
>      /**
>       * Key which has changed
>       */
>      public final Key key;
>
>      /**
>       * The status before the transition.
>       */
>      public final Status beforeStatus;
>
>      /**
>       * The after status. Note: this may be the same as the before status.
>       */
>      public final Status status;
>
>      /**
>       * Has the signature changed?
>       */
>      public final boolean isSignatureChange;
>
>      /**
>       * Either the key whose change caused the this update.  If null, this
>       * change was caused by new eval() source for the key.
>       */
>      public final Key causeKey;
>
>      /**
>       * Exception thrown while running, or null.
>       */
>      public final Exception exception;
>
>      /**
>       * The result value of successful run. The value is null if not
> executed
>       * or an exception was thrown.  The value is the empty string if
> statement
>       * or void valued expression executed.
>       */
>      public final String value;
>
>
> Yes?
>
> -Robert
>
>
> On 05/28/15 15:28, Robert Field wrote:
>>
>> On 05/28/15 13:29, Brian Goetz wrote:
>>> Sounds good!  Can you post an API draft of what you're thinking?
>>
>> The event structure is already pushed --
>>
>>     public class StatusTransitionEvent {
>>         public StatusTransitionEvent(Key key, Status beforeStatus,
>>     Status status,
>>                 boolean isSignatureChange, Key causeKey) {
>>             this.key = key;
>>             this.beforeStatus = beforeStatus;
>>             this.status = status;
>>             this.isSignatureChange = isSignatureChange;
>>             this.causeKey = causeKey;
>>         }
>>
>>         /**
>>          * Key which has changed
>>          */
>>         public final Key key;
>>
>>         /**
>>          * The status before the transition.
>>          */
>>         public final Status beforeStatus;
>>
>>         /**
>>          * The after status. Note: this may be the same as the before
>>     status.
>>          */
>>         public final Status status;
>>
>>         /**
>>          * Has the signature changed?
>>          */
>>         public final boolean isSignatureChange;
>>
>>         /**
>>          * Either the key whose change caused the this update.  If
>>     null, this
>>          * change was caused by new eval() source for the key.
>>          */
>>         public final Key causeKey;
>>     }
>>
>> It will have event registration and at least initially and probably
>> permanently a List of events would be returned by eval() -- presumably
>> with value/exception -- and by drop().
>>
>> Thoughts?
>>
>> -Robert
>>
>>>
>>>
>>>
>>> On 5/28/2015 4:28 PM, Robert Field wrote:
>>>> With keys generated for RejectedFailed and the addition of Dropped and
>>>> particularly Overwritten Status, Outcome wasn't really making any sense
>>>> any more.  There has been a desire to move in the direction of events.
>>>> The two go well together.  I've introduced StatusTransitionEvent, and
>>>> plan to shift to it.
>>>>
>>>> As soon as I can get the tests converted, I will be using
>>>> StatusTransitionEvent as the event model for update.  I will leave the
>>>> existing enum Outcome and Result.updates in place until we can convert
>>>> everything, but please consider them deprecated.
>>>>
>>>> Please let me know any concerns, questions, ...
>>>>
>>>> -Robert
>>>>
>>
>