Hello all,
I think our Promise behaves wrong, but I would like a second opinion. We have this test case:
testifRejectedRunsBlockIfPromiseFails | p q error | error := nil. p := Promise new. q := p ifRejected: [:e | error := e]. p rejectWith: KeyNotFound new. self assert: q isRejected. self assert: KeyNotFound equals: error class.
But the JavaScript behavior is actually different:
x = {}; p = Promise.reject("Rejected"); q = p.then(null, error => error); q.then(value => { x.value = value; }, error => { x.error = error }); x // <- { value: "Rejected" }
The difference: the chained promise q is resolved instead of rejected. Otherwise x would look like this: { error: "Rejected" }.
I think the behavior in our Promise is also in violation with the Promises/A+ specification it claims to implement (https://promisesaplus.com/):
[...] 2.2.7 then must return a promise.
promise2 = promise1.then(onFulfilled, onRejected);
2.2.7.1 If either onFulfilled or onRejected returns a value x, run the Promise Resolution Procedure [[Resolve]](promise2, x).
2.2.7.2 If either onFulfilled or onRejected throws an exception e, promise2 must be rejected with e as the reason. [...]
So if I understand this correctly, since in the test case Promise p gets rejected and the ifRejected: block evaluates to e without being curtailed by an exception, Promise q should be resolved, not rejected, with e, according to §2.2.7.1.
Do you agree and should we fix this?
On an only slightly related note, I am unhappy that I cannot debug errors in promise handler blocks (then: [...] ifRejected: [...]). It catches all those errors and just rejects the promise. In a server environment that would probably be ok, but at development time at least I want to have the chance to investigate the problem in a debugger. Especially those programmer's errors resulting in MessageNotUnderstood, which I could fix on the fly and let the program proceed. In JavaScript you cannot resume exceptions, but in Smalltalk you can. It might be nice to turn the on: Error do: [:e | p rejectWith: e] code in Promise into an ifCurtailed: [p rejectWith: e] to allow exceptions/errors to be resumed before rejecting the promise. The promise would still be rejected if you press Abandon in the debugger. Problem: further errors in the rejected handlers would result in "Unwind error during termination". This could in turn be avoided by rejecting the promise in the "future", that is the next cycle of the world. Opinions?
Kind regards, Jakob
Dear Jakob,
we had to deal with Promises a lot in a project and ended up creating a copy of the current Squeak promises to adjust some of their behavior, so great that you're bringing it up again! You can find our changed version here: https://github.com/tom95/Pheno/tree/master/src/Pheno-Core.package/PHPromise....
The example should also work as stated in the spec:
p := PHPromise new. q := p ifRejected: [:e | 5]. p rejectWith: PHTestApplicationException new. q " a PHPromise(resolved: 5) "
Further, we introduced a differentiation between "application errors" and "system errors". It made sense for our usecase, since we were building a GUI application, where things like failed constraints on an update caused a nice user-readable error that was then considered an application error and caught by the ifError: statement. All other errors, however, we signaled right away, since usually this ended up making debugging routines a lot more complicated when each MNU got hidden away. This is also why I am throwing a PHTestApplicationException, rather than the KeyNotFound, in the example above: it overrides `isApplicationError` to return true while `Exception` returns false.
This differentiation worked very well for us, it might be a bit confusing otherwise though, since it may not be exactly what you would expect considering the spec. I would also be happy to adopt the solution that browsers took, i.e. only throwing an exception when there's no registered handlers, and otherwise making the caught exceptions more debuggable of course.
Best, Tom ________________________________________ From: Squeak-dev squeak-dev-bounces@lists.squeakfoundation.org on behalf of Jakob Reschke forums.jakob@resfarm.de Sent: Sunday, April 5, 2020 12:00 AM To: squeak-dev@lists.squeakfoundation.org Subject: [squeak-dev] Error in Promises/A+ reject implementation?
Hello all,
I think our Promise behaves wrong, but I would like a second opinion. We have this test case:
testifRejectedRunsBlockIfPromiseFails | p q error | error := nil. p := Promise new. q := p ifRejected: [:e | error := e]. p rejectWith: KeyNotFound new. self assert: q isRejected. self assert: KeyNotFound equals: error class.
But the JavaScript behavior is actually different:
x = {}; p = Promise.reject("Rejected"); q = p.then(null, error => error); q.then(value => { x.value = value; }, error => { x.error = error }); x // <- { value: "Rejected" }
The difference: the chained promise q is resolved instead of rejected. Otherwise x would look like this: { error: "Rejected" }.
I think the behavior in our Promise is also in violation with the Promises/A+ specification it claims to implement (https://promisesaplus.com/):
[...] 2.2.7 then must return a promise.
promise2 = promise1.then(onFulfilled, onRejected);
2.2.7.1 If either onFulfilled or onRejected returns a value x, run the Promise Resolution Procedure [[Resolve]](promise2, x).
2.2.7.2 If either onFulfilled or onRejected throws an exception e, promise2 must be rejected with e as the reason. [...]
So if I understand this correctly, since in the test case Promise p gets rejected and the ifRejected: block evaluates to e without being curtailed by an exception, Promise q should be resolved, not rejected, with e, according to §2.2.7.1.
Do you agree and should we fix this?
On an only slightly related note, I am unhappy that I cannot debug errors in promise handler blocks (then: [...] ifRejected: [...]). It catches all those errors and just rejects the promise. In a server environment that would probably be ok, but at development time at least I want to have the chance to investigate the problem in a debugger. Especially those programmer's errors resulting in MessageNotUnderstood, which I could fix on the fly and let the program proceed. In JavaScript you cannot resume exceptions, but in Smalltalk you can. It might be nice to turn the on: Error do: [:e | p rejectWith: e] code in Promise into an ifCurtailed: [p rejectWith: e] to allow exceptions/errors to be resumed before rejecting the promise. The promise would still be rejected if you press Abandon in the debugger. Problem: further errors in the rejected handlers would result in "Unwind error during termination". This could in turn be avoided by rejecting the promise in the "future", that is the next cycle of the world. Opinions?
Kind regards, Jakob
Hi Jakob,
On 4/5/20 12:00 AM, Jakob Reschke wrote:
I think our Promise behaves wrong, but I would like a second opinion.
I think you're right. And I think we should fix it. I'll draft a repair.
I don't think there are many users of Promise out there -- if there are, and any are reading this, please speak up if fixing the bug Jakob reported would be a problem.
I think the behavior in our Promise is also in violation with the Promises/A+ specification it claims to implement (https://promisesaplus.com/):
[...] 2.2.7 then must return a promise.
promise2 = promise1.then(onFulfilled, onRejected);2.2.7.1 If either onFulfilled or onRejected returns a value x, run the Promise Resolution Procedure [[Resolve]](promise2, x).
2.2.7.2 If either onFulfilled or onRejected throws an exception e, promise2 must be rejected with e as the reason. [...]
Thanks for digging into the spec here. That'll be helpful for getting the tests right.
On an only slightly related note, I am unhappy that I cannot debug errors in promise handler blocks [...] Opinions?
I don't know what to think about this, I'm afraid! I'd be keen to hear more from others about what a good alternative to the current behaviour might be.
Regards, Tony
Hi Tony,
Well, this Promise implementation made it into the Kernel and schedules into the future (this seems to be the only use). So it certainly strikes me as official.
Have you ever had a chance to look at the 'CapabilitiesLocal' implementation I modeled after ERights, many many years ago? It has tests. I would welcome comment & feedback.
Installer ss project: 'Cryptography'; install: 'CapabilitiesLocal'.
Kindly, Robert
On 6/4/20 10:16 AM, Tony Garnock-Jones wrote:
Hi Jakob,
On 4/5/20 12:00 AM, Jakob Reschke wrote:
I think our Promise behaves wrong, but I would like a second opinion.
I think you're right. And I think we should fix it. I'll draft a repair.
I don't think there are many users of Promise out there -- if there are, and any are reading this, please speak up if fixing the bug Jakob reported would be a problem.
I think the behavior in our Promise is also in violation with the Promises/A+ specification it claims to implement (https://promisesaplus.com/):
[...] 2.2.7 then must return a promise.
promise2 = promise1.then(onFulfilled, onRejected);2.2.7.1 If either onFulfilled or onRejected returns a value x, run the Promise Resolution Procedure [[Resolve]](promise2, x).
2.2.7.2 If either onFulfilled or onRejected throws an exception e, promise2 must be rejected with e as the reason. [...]
Thanks for digging into the spec here. That'll be helpful for getting the tests right.
On an only slightly related note, I am unhappy that I cannot debug errors in promise handler blocks [...] Opinions?
I don't know what to think about this, I'm afraid! I'd be keen to hear more from others about what a good alternative to the current behaviour might be.
Regards, Tony
Hi Tony,
I reworked the packaging and the code split. Please load the following to see local promises:
Installer ss project: 'Cryptography'; install: 'PromisesLocal'.
and then run the PromisesLocal tests, all #green.
---
And to see the remote promises, load the following:
Installer ss project: 'Cryptography'; install: 'ProCrypto-1-1-1'; install: 'ProCryptoTests-1-1-1'. Installer ss project: 'Cryptography'; install: 'PromisesLoader'.
Run all tests, EXCEPT PromisesRemote tests. 414 tests, all #green. Now run the following #green PromisesRemote tests:
- Base64EncodingTestCase
- RememberTableTestCase
- SealerUnsealerTestCase
- SerializationTestCase
- SubstitutionSerializationTestCase
As I am porting PromisesRemote onto the latest iteration of the ParrotTalk interface, the remote tests (LookupTestCase & OperationalTestCase) are not green. They're #Red. I wanted to give you a heads up on the package renames for local promises, such that you could not waste your time exploring this implementation. I'll email once I get those remote tests working again.
K, r
On 6/4/20 5:25 PM, Robert Withers wrote:
Hi Tony,
Well, this Promise implementation made it into the Kernel and schedules into the future (this seems to be the only use). So it certainly strikes me as official.
Have you ever had a chance to look at the 'CapabilitiesLocal' implementation I modeled after ERights, many many years ago? It has tests. I would welcome comment & feedback.
Installer ss project: 'Cryptography'; install: 'CapabilitiesLocal'.
Kindly, Robert
On 6/4/20 10:16 AM, Tony Garnock-Jones wrote:
Hi Jakob,
On 4/5/20 12:00 AM, Jakob Reschke wrote:
I think our Promise behaves wrong, but I would like a second opinion.
I think you're right. And I think we should fix it. I'll draft a repair.
I don't think there are many users of Promise out there -- if there are, and any are reading this, please speak up if fixing the bug Jakob reported would be a problem.
I think the behavior in our Promise is also in violation with the Promises/A+ specification it claims to implement ( https://promisesaplus.com/ ):
[...] 2.2.7 then must return a promise.
promise2 = promise1.then(onFulfilled, onRejected);2.2.7.1 If either onFulfilled or onRejected returns a value x, run the Promise Resolution Procedure [[Resolve]](promise2, x).
2.2.7.2 If either onFulfilled or onRejected throws an exception e, promise2 must be rejected with e as the reason. [...]
Thanks for digging into the spec here. That'll be helpful for getting the tests right.
On an only slightly related note, I am unhappy that I cannot debug errors in promise handler blocks [...] Opinions?
I don't know what to think about this, I'm afraid! I'd be keen to hear more from others about what a good alternative to the current behaviour might be.
Regards, Tony
Alright, I have updated ThunkStack and PromisesRemote. Please load with the script:
Installer ss project: 'Cryptography'; install: 'ProCrypto-1-1-1'; install: 'ProCryptoTests-1-1-1'. Installer ss project: 'Cryptography'; install: 'PromisesLoader'.
and run tests, including the test category: PromisesRemote-Testing. #Green! This includes remote tests between two machines {vats}.
The test category: PromisesRemote-3WayTesting has two failures due to 3Way no working right, yet. Here are the two failing tests:
- testThreeWayGranovetter
- testThreeWayGranovetterGalaxyScenario
I am buried in SSL, but I will try to advance PromisesRemote by switching from STON to ASN1 encoding.
K, r
On 6/5/20 10:43 AM, Robert Withers wrote:
Hi Tony,
I reworked the packaging and the code split. Please load the following to see local promises:
Installer ss project: 'Cryptography'; install: 'PromisesLocal'.
and then run the PromisesLocal tests, all #green.
And to see the remote promises, load the following:
Installer ss project: 'Cryptography'; install: 'ProCrypto-1-1-1'; install: 'ProCryptoTests-1-1-1'. Installer ss project: 'Cryptography'; install: 'PromisesLoader'.
Run all tests, EXCEPT PromisesRemote tests. 414 tests, all #green. Now run the following #green PromisesRemote tests:
- Base64EncodingTestCase
- RememberTableTestCase
- SealerUnsealerTestCase
- SerializationTestCase
- SubstitutionSerializationTestCase
As I am porting PromisesRemote onto the latest iteration of the ParrotTalk interface, the remote tests (LookupTestCase & OperationalTestCase) are not green. They're #Red. I wanted to give you a heads up on the package renames for local promises, such that you could not waste your time exploring this implementation. I'll email once I get those remote tests working again.
K, r
On 6/4/20 5:25 PM, Robert Withers wrote:
Hi Tony,
Well, this Promise implementation made it into the Kernel and schedules into the future (this seems to be the only use). So it certainly strikes me as official.
Have you ever had a chance to look at the 'CapabilitiesLocal' implementation I modeled after ERights, many many years ago? It has tests. I would welcome comment & feedback.
Installer ss project: 'Cryptography'; install: 'CapabilitiesLocal'.
Kindly, Robert
On 6/4/20 10:16 AM, Tony Garnock-Jones wrote:
Hi Jakob,
On 4/5/20 12:00 AM, Jakob Reschke wrote:
I think our Promise behaves wrong, but I would like a second opinion.
I think you're right. And I think we should fix it. I'll draft a repair.
I don't think there are many users of Promise out there -- if there are, and any are reading this, please speak up if fixing the bug Jakob reported would be a problem.
I think the behavior in our Promise is also in violation with the Promises/A+ specification it claims to implement ( https://promisesaplus.com/ ):
[...] 2.2.7 then must return a promise.
promise2 = promise1.then(onFulfilled, onRejected);2.2.7.1 If either onFulfilled or onRejected returns a value x, run the Promise Resolution Procedure [[Resolve]](promise2, x).
2.2.7.2 If either onFulfilled or onRejected throws an exception e, promise2 must be rejected with e as the reason. [...]
Thanks for digging into the spec here. That'll be helpful for getting the tests right.
On an only slightly related note, I am unhappy that I cannot debug errors in promise handler blocks [...] Opinions?
I don't know what to think about this, I'm afraid! I'd be keen to hear more from others about what a good alternative to the current behaviour might be.
Regards, Tony
squeak-dev@lists.squeakfoundation.org
-
Beckmann, Tom -
Jakob Reschke -
Robert Withers -
Tony Garnock-Jones