Fix child SEFF IC #69
Conversation
stiesssh
changed the title
There was a problem hiding this comment.
Changes seem reasonable, it gets rid of unneccesary pointers to callers from within child contexts. Further it fixes the bug when interpreting case ExternalCallAction where the caller makes itself a caller in its interpretation context. (L224:SeffInterpreter).
I requested several changes that contribute to making the code cleaner (e.g., removing some commented lines entirely) and also documenting a bit better in the SIC the relation to the COW request.
| .orElseThrow(() -> new IllegalStateException( | ||
| "Since the call came from somewhere else, the context of the caller must be present, but it isn't.")); | ||
| final SEFFInterpretationContext seffInterpretationContext = entity.getRequestFrom(); | ||
| // .orElseThrow(() -> new IllegalStateException( |
| * | ||
| * A context's {@code callOverWireRequest} must <b>never</b> have their | ||
| * {@code replyTo} set. If {@code replyTo} is set, it is a return to a caller, | ||
| * and a return cannot be the calling request. |
There was a problem hiding this comment.
This comment does not help at this point. It seems more like a thought that the developer did not know where to document/put.
The comment talks about a private and encapsulated attribute of the object that this class uses.
Further the sentence "A context's {@code callOverWireRequest} must never have their
- {@code replyTo} set." is not true. If you say upon construction then its true.
There was a problem hiding this comment.
Actually, i think it is true. But maybe the formulation is inconcise, i reformulated and moved it.
| * | ||
| * TODO Children carry the {@code calledFrom} of their parents, but not their | ||
| * {@code callOverWireRequest}. However i think, they should only appear | ||
| * together. [S3] |
There was a problem hiding this comment.
Why is here a TODO?
I suggest merging the three paragraphs into one paragraph that concisely elaborates the relationship between the SEFFInterpretationContext and the callOverWireRequest.
There was a problem hiding this comment.
i created an issue for the TODO an moved the other comments to there respective attributes.
Issue: #76 while i was already at it, i also included you comment regarding the CallOverWireRequest, such that we do not forget about them.
| assert builder.callOverWireRequest == null | ||
| || (builder.callOverWireRequest != null && builder.calledFrom.isPresent()) | ||
| : String.format("Missing caller in %s", this.getClass().getSimpleName()); | ||
| // || (builder.callOverWireRequest == null && builder.calledFrom.isEmpty()): |
| @@ -176,6 +188,7 @@ public Builder withParent(final SEFFInterpretationContext parent) { | |||
| } | |||
|
|
|||
| public Builder withCallOverWireRequest(final CallOverWireRequest callOverWireRequest) { | |||
| assert callOverWireRequest == null || callOverWireRequest.getReplyTo().isEmpty(); | |||
There was a problem hiding this comment.
why not simply builderNonNull(cowr). Why is it important to check from the perspective of the seff ic that the reply to is empty?
There was a problem hiding this comment.
Because a SEFF must not be called by a reply-cowr. I.e. if i check whether replyTo is empty, i ensure, that i only use cowr that are actually calling a SEFF.
basically, it's just a check to prevent mishaps in case we change code later on.
| @@ -126,7 +126,8 @@ public Set<SEFFInterpreted> caseBranchAction(final BranchAction branchAction) { | |||
| final SEFFInterpretationContext childContext = this.context.createChildContext() | |||
| .withBehaviorContext(holder) | |||
| .withRequestProcessingContext(this.context.getRequestProcessingContext()) | |||
| .withCaller(this.context.getCaller()) | |||
| // .withCaller(this.context.getCaller()) // imho, children should not have | |||
| // callers, only parents. | |||
Current
Caller Self-Reference
SEFFInterpretationContexts (SEFFIC) that contain anExternalCallActionget updated to be their own caller, such that we end up with object relations like this:PCM model, colours correpond to the SEFF ICs.
All blue SEFFICs are copies of each other, created when calling
update. Note, that the copies reference the original as caller which sematically makes zero sense.Reset of
CallOverwireRequestinonCallOverWireSucceededWhen returning from a call the callOverwireRequest (COWR) of the caller gets reset to
null. This is wrong, because semantically the COWR of a SEFFIC is the request, by which the SEFFIC was called. The example given above works just fine, but in case of nested external calls (see figure below), returning from the SEFFIC that is both callee and caller (green) to its caller breaks.New
Caller Self-Reference
Copied SEFFICs do no longer reference their original as caller. As a follow up, the "unwrapping" of SEFFIC was adapted as well.
object relation is now like this:
Reset of
CallOverwireRequestinonCallOverWireSucceededRemoved the reset. Now even nested external calls can return to their caller.
Misc
StackContext. However, theStackContextis broken anyway, thus i suggest to ignore the Stack for this PR and fix it separately after merging this PR.