Added Breaking/Erroring of Promises #118

salenaer · 2017-03-13T16:25:52Z

I've added ruining of promises, currently, an error occurs in the tests, but not during regular use. Neither Carmen nor I find the reason of the error.
Can you have a look at the code and see if we missed something?

Design: Ruining Promise in Absence of Handlers

A message is sent from A to B. The evaluation of this message in B results in an error. As such the promise (the result of this message send in A) needs to be ruined. To ruin a promise means setting the state to erroneous and running all error handlers connected to this promise. This exposes the occurrence of the error to A.

First, the handlers bound to this promise are started. If no such handlers exist the chained promises (promises resolved with the current ruined promise) are ruined. If none of the chained promises handled the exception, this exception is uncaught. Normally the user should be informed when an uncaught exception occurs. However, in our case, this is impossible, because of scheduling it is possible that the promise is ruined before the handlers are added. This is a race condition, this race condition should not be exposed to the user.

Pseudocode example of race condition as a result of scheduling, time runs downwards

A 						B
promise := B <-: message

						process(message)


promise whenResolved: [something] onError: [ handlers]

To change this behaviour make ruinChainedPromise return a handled boolean. Based on this boolean perform desired behaviour inside onError method of SPromise.

smarr

Thanks a lot for the pull request. This is a good start. However, I would have much preferred if we could have discussed some of the details like naming upfront.

The 'ruin' terminology does not fit into SOMns, it is likely AmbientTalk specific. Please either use 'break' as in Newspeak, or 'error' as it is already named in SOMns. Changing terminology is a major change that I am not happy to accept without proper discussion.

The other concerns I have are with commented tests, and what looks like duplication of rather complicated code.

Please let me know whether you would be happy to do those changes, or whether you would prefer me to take it from here.

smarr · 2017-03-13T16:55:42Z

core-lib/Actors.som

-    public resolve: value = ( vmMirror actorsResolve: self with: value isBPResolution: false)
-    public error:   value = ( self notYetImplemented )
+    public resolve: value = (vmMirror actorsResolve: self with: value isBPResolution: false)
+    public error:   value = (vmMirror actorsRuin: self with: value)


I am not happy with the terminology. ruin is neither SOMns nor Newspeak terminology.
SOMns uses error:, Newspeak uses whenBroken:. I would be willing to consider Newspeak terminology, but not ruin.

smarr · 2017-03-13T16:55:59Z

core-lib/Actors.som

@@ -98,6 +98,11 @@ class Actors usingVmMirror: vmMirror usingKernel: kernel = Value (
    public resolve: value = (
      resolver resolve: value.
    )
+
+    (*added*)
+    public ruin: exception = (


Same here, ruin: is a no go. And anywhere in the code below.

smarr · 2017-03-13T16:57:57Z

core-lib/TestSuite/ActorTests.som

+      )
+    )
+
+    (*


Why is this test commented out?

smarr · 2017-03-13T16:58:11Z

core-lib/TestSuite/ActorTests.som

+    ) *)
+
+  (*    
+    public testAsyncPromiseOfPromiseResolvesToException = (


Same here, why adding commented out code?

Most of the tests are commented out as they do work when ran on their own. When I run multiple at the same time I get some error I can't seem to solve.

Ok, so, just to clarify, do you want me to change the things I currently marked as issues and also try to solve the remaining bugs? I could do that. But I don't want to just take it out of your hands, since you probably spend already a lot of sweat on the PR.

You can take over, I really need to focus on things I need for my thesis

smarr · 2017-03-13T16:58:42Z

core-lib/TestSuite/ActorTests.som

+      pp resolver error: exception.
+
+      ^ assertPromise
+    ) *)


Is there any test that's actually not commented out?

smarr · 2017-03-13T17:06:42Z

src/som/interpreter/actors/SPromise.java

-    public final void onError() {
-      throw new NotYetImplementedException(); // TODO: implement
+    public static void onError(final SAbstractObject exception, final Object wrapped, final SPromise p, final Actor current) {
+      //System.out.println("called on error");


Please don't commit commented debugging code.

smarr · 2017-03-13T17:10:24Z

src/som/interpreter/actors/SPromise.java

+        //execute all exceptionHandlers if the type of wrapped is equal to the exceptionClass the handler handles.
+        if (p.onException != null) {
+          for (int i = 0; i < p.onException.size(); i++) {
+            SClass exceptionClass = p.onException.get(i);


I was under the impression that we figured out that the onException stuff can't be supported because it implies visible race conditions. Or is this not the case after all?
I guess it depends on how this is realized. But from the sequential world, filtering would be the way to go, which is a race. So, it can't be done, I think.

I'd rather remove the on: exception support.

smarr · 2017-03-13T17:14:12Z

src/som/interpreter/actors/SPromise.java

+        return handled;
+    }
+
+    protected static void ruinChainedPromisesUnsynch(final SAbstractObject exception, final SPromise promise, final Actor current){


Would it be possible to use the existing code here instead, and pass as an argument whether the resolution is successful or erroneous? This seems to be a duplication of rather intricate code, which I'd really like to avoid.

smarr · 2017-03-13T17:14:32Z

src/som/primitives/actors/PromisePrims.java

@@ -182,6 +182,26 @@ public final SPromise whenResolvedOnError(final SPromise promise,
      return whenResolvedOrError(promise, resolved, error, resolvedTarget,
          errorTarget, registerNode);
    }
+


Please don't included dead code

smarr · 2017-03-13T17:16:00Z

thesis/behaviour.som

@@ -0,0 +1,45 @@
+class Behaviour usingPlatform: platform = Value (


Are the following files really to be committed here? seems like something you'd like in a separate repo.
Considering that the folder is named thesis?

I want them in my local fork and thought you could refuse them when merging with the main.

Ok, you might want to put them in a separate repo to avoid issues, since you seem to have sent the PR from master, I am worried that if i delete them, you might just miss them.

smarr · 2017-03-14T08:20:09Z

Ok, thanks, then I'll take it over. And thanks for taking care of the checkstyle issues, I really appreciate it. I know it is tedious but helps to maintain a more uniform code style.

I'll push a rebase on master and then try to figure out what's going on with the tests.

smarr · 2017-03-14T20:07:27Z

@salenaer thanks again for the pull request.
And apologies for many of the issues that you saw. Some of that is because of broken stuff in the test framework. For instance: 79e293c

As you may see from the updated PR, I tried to avoid much of the code duplication of the previous version by reusing all of the normal resolution code. Instead of duplicating it, it now got a parameter that defines success or failure. Beside that, I only replicated a few optimizations that were in place before.

After a few minor fixes to the tests, they pass.

Would be great, if you could review the PR and check whether I missed anything.

Thanks!

smarr · 2017-03-14T20:07:58Z

And yeah, that's just another example for: everything that's not tested is broken. Sorry...

smarr · 2017-03-15T07:55:34Z

src/som/primitives/actors/PromisePrims.java


    public WhenResolvedOnErrorPrim(final boolean eagWrap, final SourceSection source) { super(eagWrap, source); }

-    @Specialization(guards = {"resolvedMethod == resolved.getMethod()", "errorMethod == error.getMethod()"})
+    @Specialization(guards = {"resolvedMethod == resolved.getMethod()", "errorMethod == error.getMethod()"}, limit = "1000")


Todo for myself: remove limit, and replace by proper fallback case

2 unresolved issues: -actors.som (seems to be redundent) -what to do with an exception that is not caught.

- adding tests for ruining - fixed explicit ruining - found problem with chaining and ruining RM tiny Signed-off-by: Stefan Marr <git@stefan-marr.de>

- added tests for ruining - made ruining ruin all chained if not handled - made ruining ruin chained of own chained if not handled - currently bug causing two test to crash if run consequently - added four more test - finished nested ruining, tests still fail RM behavior Signed-off-by: Stefan Marr <git@stefan-marr.de>

- checkstyle errors - codacy errors

- execution stats are kept in TracingActivityThread - chainedPromise seems to need frame for now - avoid using ‘ruin’, prefer ‘error’ or ‘break’ (work in progress) - trace broken promise like resolved ones Signed-off-by: Stefan Marr <git@stefan-marr.de>

The #on:do: method cannot be properly supported for promises, because registering handlers with filtering semantics is a racy operation. Depending on the order of events, a handler might or might not trigger because a filter might have been inplace. And, we don’t want to expose such races. Signed-off-by: Stefan Marr <git@stefan-marr.de>

Signed-off-by: Stefan Marr <git@stefan-marr.de>

- revise them, still were using old naming - adapt assert in Minitest, too Signed-off-by: Stefan Marr <git@stefan-marr.de>

Signed-off-by: Stefan Marr <git@stefan-marr.de>

- fix up terminology to avoid ruin - fix some simple issues - fix code style Signed-off-by: Stefan Marr <git@stefan-marr.de>

Signed-off-by: Stefan Marr <git@stefan-marr.de>

- avoid code duplication, reusued normal resolution, and use Resolution as argument to distinguish - introduce common AbstractPromiseResolutionNode to reuse code between successful and erroneous resolution - use RegisterOnError node to optimize callback registration, same as RegisterWhenResolved - check resolution type when resolving callbacks - propagate breakpoint info - use same implementation strategy for OnErrorPrim as for normal WhenResolvedPrim - add promise error parsing to trace parsing Signed-off-by: Stefan Marr <git@stefan-marr.de>

Signed-off-by: Stefan Marr <git@stefan-marr.de>

smarr · 2017-03-15T16:43:37Z

@salenaer the PR is merged and on master now. Thanks.

smarr changed the title ~~Runing~~ Added Ruining of Promises Mar 13, 2017

smarr self-assigned this Mar 13, 2017

smarr added this to the v0.3.0 milestone Mar 13, 2017

smarr added bug Fixes an issue, incorrect implementation enhancement Improves the implementation with something noteworthy labels Mar 13, 2017

smarr requested changes Mar 13, 2017

View reviewed changes

smarr mentioned this pull request Mar 13, 2017

Catch exceptions for implicit promises #74

Closed

smarr force-pushed the master branch 2 times, most recently from 1c81786 to 02f8414 Compare March 14, 2017 20:00

smarr force-pushed the master branch from 02f8414 to 857d647 Compare March 14, 2017 21:59

smarr reviewed Mar 15, 2017

View reviewed changes

smarr force-pushed the master branch 2 times, most recently from dd81f26 to 049256f Compare March 15, 2017 11:17

salenaer and others added 14 commits March 15, 2017 17:25

Add ruining/breaking of promises

bc84f4b

2 unresolved issues: -actors.som (seems to be redundent) -what to do with an exception that is not caught.

Added basic tracing or ruining/breaking of promies (untested)

8465991

- adding tests for ruining - fixed explicit ruining - found problem with chaining and ruining RM tiny Signed-off-by: Stefan Marr <git@stefan-marr.de>

Changed errornous to erroneous

6c6cd8e

Fix code style issues

f5dbbbb

- checkstyle errors - codacy errors

Remove dead code

fd74c1e

Signed-off-by: Stefan Marr <git@stefan-marr.de>

Enable old tests, with Newspeak background

33dc3b1

- revise them, still were using old naming - adapt assert in Minitest, too Signed-off-by: Stefan Marr <git@stefan-marr.de>

Fix broken error handling in Minitests for broken promises

931a125

Signed-off-by: Stefan Marr <git@stefan-marr.de>

Revise promise interface and documentation

3e93afc

Signed-off-by: Stefan Marr <git@stefan-marr.de>

Remove code that’s duplicated for error case

cfd7333

Signed-off-by: Stefan Marr <git@stefan-marr.de>

Enable promise erroring tests

ef1d04a

- fix up terminology to avoid ruin - fix some simple issues - fix code style Signed-off-by: Stefan Marr <git@stefan-marr.de>

Fix parse errors in Actors.som

c3808db

Signed-off-by: Stefan Marr <git@stefan-marr.de>

smarr added 5 commits March 15, 2017 17:25

Fix parse errors and bugs in ActorTests

5e9f442

Signed-off-by: Stefan Marr <git@stefan-marr.de>

Fix bug in CachedDnuNode, didn’t set nextInCache

4b78b05

Signed-off-by: Stefan Marr <git@stefan-marr.de>

Updated Dynamic Metrics tool test data after changes to core-lib

1c09955

Signed-off-by: Stefan Marr <git@stefan-marr.de>

Delete old all.gcov file when updating coverage data

9ad0d10

Signed-off-by: Stefan Marr <git@stefan-marr.de>

smarr force-pushed the master branch from 049256f to 9ad0d10 Compare March 15, 2017 16:26

smarr changed the title ~~Added Ruining of Promises~~ Added Breaking/Erroring of Promises Mar 15, 2017

Added CHANGELOG entry

ed393bc

Signed-off-by: Stefan Marr <git@stefan-marr.de>

smarr merged commit 9c9888f into smarr:master Mar 15, 2017

smarr mentioned this pull request Mar 15, 2017

There is an issue with failing asynchronous tests. Harness fails with SomException instead of reporting the error #2

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Breaking/Erroring of Promises #118

Added Breaking/Erroring of Promises #118

salenaer commented Mar 13, 2017 •

edited by smarr

Loading

smarr left a comment

smarr Mar 13, 2017

smarr Mar 13, 2017

smarr Mar 13, 2017

smarr Mar 13, 2017

salenaer Mar 13, 2017

smarr Mar 13, 2017

salenaer Mar 14, 2017

smarr Mar 13, 2017

smarr Mar 13, 2017

smarr Mar 13, 2017

smarr Mar 13, 2017

smarr Mar 13, 2017

smarr Mar 13, 2017

salenaer Mar 13, 2017

smarr Mar 13, 2017

smarr commented Mar 14, 2017

smarr commented Mar 14, 2017 •

edited

Loading

smarr commented Mar 14, 2017

smarr Mar 15, 2017

smarr commented Mar 15, 2017

+                    )
+                  )
+                  (*

		@@ -0,0 +1,45 @@
		class Behaviour usingPlatform: platform = Value (

Added Breaking/Erroring of Promises #118

Added Breaking/Erroring of Promises #118

Conversation

salenaer commented Mar 13, 2017 • edited by smarr Loading

Design: Ruining Promise in Absence of Handlers

smarr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarr commented Mar 14, 2017

smarr commented Mar 14, 2017 • edited Loading

smarr commented Mar 14, 2017

Choose a reason for hiding this comment

smarr commented Mar 15, 2017

salenaer commented Mar 13, 2017 •

edited by smarr

Loading

smarr commented Mar 14, 2017 •

edited

Loading