Skip to content

Commit e3f7d5e

Browse files
comcalvicomcalvi
authored
feat(python): add error differentiation to Python. (#3772)
Uses `RuntimeError` and `JsiiError` to indicate recoverable vs non-recoverable errors respectively. Also fixes three bugs in the Java logic for the same feature: * Callbacks did not pass the error type over the wire, meaning that some faults were rethrown as runtime errors and vice versa * async method invocations did not pass the error type over the wire, meaning that some faults were rethrown as runtime errors and vice versa. * the java enum used `RuntimeException` in the string where it should have used `RuntimeError`, meaning that some errors would always be rethrown as `RuntimeException`s even when they should be `JsiiError`s. These bugs happened because the Java tests did not check for type of the exception thrown, meaning that `JsiiError`s could be passed where `RuntimeException`s were expected. The tests now verify the type of the exception thrown. --- By submitting this pull request, I confirm that my contribution is made under the terms of the [Apache 2.0 license]. [Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0
1 parent 4fd370b commit e3f7d5e

File tree

13 files changed

+123
-29
lines changed

13 files changed

+123
-29
lines changed

‎packages/@jsii/java-runtime-test/project/src/test/java/software/amazon/jsii/testing/ComplianceTest.java‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -333,9 +333,12 @@ public void exceptions() {
333333
assertEquals(23, calc3.getValue());
334334
boolean thrown = false;
335335
try {
336-
calc3.add(10);
336+
calc3.add(10);
337+
} catch (JsiiException e) {
338+
// We expect a RuntimeException that is NOT a JsiiException.
339+
throw e;
337340
} catch (RuntimeException e) {
338-
thrown = true;
341+
thrown = true;
339342
}
340343
assertTrue(thrown);
341344
calc3.setMaxValue(40);
@@ -450,6 +453,8 @@ public java.lang.Number overrideMe(java.lang.Number mult) {
450453
boolean thrown = false;
451454
try {
452455
obj.callMe();
456+
} catch (JsiiException e) {
457+
throw e;
453458
} catch (RuntimeException e) {
454459
assertTrue(e.getMessage().contains( "Thrown by native code"));
455460
thrown = true;
@@ -519,6 +524,8 @@ public String getTheProperty() {
519524
boolean thrown = false;
520525
try {
521526
so.retrieveValueOfTheProperty();
527+
} catch (JsiiException e) {
528+
throw e;
522529
} catch (RuntimeException e) {
523530
assertTrue(e.getMessage().contains("Oh no, this is bad"));
524531
thrown = true;
@@ -537,6 +544,8 @@ public void setTheProperty(String value) {
537544
boolean thrown = false;
538545
try {
539546
so.modifyValueOfTheProperty("Hii");
547+
} catch (JsiiException e) {
548+
throw e;
540549
} catch (RuntimeException e) {
541550
assertTrue(e.getMessage().contains("Exception from overloaded setter"));
542551
thrown = true;

‎packages/@jsii/java-runtime-test/project/src/test/java/software/amazon/jsii/testing/JsiiClientTest.java‎

Lines changed: 39 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,7 @@ public void asyncMethodOverrides() {
117117
assertEquals(obj.getObjId(), JsiiObjectRef.parse(first.getInvoke().getObjref()).getObjId());
118118

119119
// now complete the callback with some override value
120-
client.completeCallback(first, null, toSandbox(999));
120+
client.completeCallback(first, null, null, toSandbox(999));
121121

122122
// end the async invocation, but now we expect the value to be different since we override the method.
123123
JsonNode result = client.endAsyncMethod(promise);
@@ -146,19 +146,55 @@ public void asyncMethodOverridesThrow() {
146146
assertEquals(obj.getObjId(), JsiiObjectRef.parse(first.getInvoke().getObjref()).getObjId());
147147

148148
// now complete the callback with an error
149-
client.completeCallback(first, "Hello, Error", null);
149+
client.completeCallback(first, "Hello, Error", null, null);
150150

151151
// end the async invocation, but now we expect the value to be different since we override the method.
152152
boolean thrown = false;
153153
try {
154154
client.endAsyncMethod(promise);
155-
} catch (JsiiException e) {
155+
} catch (RuntimeException e) {
156+
assertEquals(RuntimeException.class, e.getClass());
156157
assertTrue(e.getMessage().contains("Hello, Error"));
157158
thrown = true;
158159
}
159160
assertTrue(thrown);
160161
}
161162

163+
@Test
164+
public void asyncMethodOverridesThrowWithFault() {
165+
JsiiObjectRef obj = client.createObject("jsii-calc.AsyncVirtualMethods", Arrays.asList(), methodOverride("overrideMe", "myCookie"), Arrays.asList());
166+
167+
// begin will return a promise
168+
JsiiPromise promise = client.beginAsyncMethod(obj, "callMe", toSandboxArray());
169+
assertFalse(promise.getPromiseId().isEmpty());
170+
171+
// now we expect to see a callback to "overrideMe" in the pending callbacks queue
172+
173+
List<Callback> callbacks = client.pendingCallbacks();
174+
175+
assertEquals(1, callbacks.size());
176+
177+
Callback first = callbacks.get(0);
178+
assertEquals("overrideMe", first.getInvoke().getMethod());
179+
assertEquals("myCookie", first.getCookie());
180+
assertEquals(1, first.getInvoke().getArgs().size());
181+
assertEquals(JsiiObjectMapper.valueToTree(10), first.getInvoke().getArgs().get(0));
182+
assertEquals(obj.getObjId(), JsiiObjectRef.parse(first.getInvoke().getObjref()).getObjId());
183+
184+
// now complete the callback with an error
185+
client.completeCallback(first, "Hello, Fault", "@jsii/kernel.Fault", null);
186+
187+
// end the async invocation, but now we expect the value to be different since we override the method.
188+
boolean thrown = false;
189+
try {
190+
client.endAsyncMethod(promise);
191+
} catch (JsiiException e) {
192+
assertTrue(e.getMessage().contains("Hello, Fault"));
193+
thrown = true;
194+
}
195+
assertTrue(thrown);
196+
}
197+
162198
@Test
163199
public void syncVirtualMethods() {
164200
JsiiObjectRef obj = client.createObject("jsii-calc.SyncVirtualMethods", Arrays.asList(), methodOverride("virtualMethod","myCookie"), Arrays.asList());

‎packages/@jsii/java-runtime/project/src/main/java/software/amazon/jsii/JsiiClient.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -243,12 +243,14 @@ public List<Callback> pendingCallbacks() {
243243
* Completes a callback.
244244
* @param callback The callback to complete.
245245
* @param error Error information (or null).
246+
* @param name Error type (or null).
246247
* @param result Result (or null).
247248
*/
248-
public void completeCallback(final Callback callback, final String error, final JsonNode result) {
249+
public void completeCallback(final Callback callback, final String error, final String name, final JsonNode result) {
249250
ObjectNode req = makeRequest("complete");
250251
req.put("cbid", callback.getCbid());
251252
req.put("err", error);
253+
req.put("name", name);
252254
req.set("result", result);
253255

254256
this.runtime.requestResponse(req);

‎packages/@jsii/java-runtime/project/src/main/java/software/amazon/jsii/JsiiEngine.java‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -486,7 +486,15 @@ private Object invokeMethod(final Object obj, final Method method, final Object.
486486
throw e;
487487
}
488488
} catch (InvocationTargetException e) {
489-
throw new JsiiError(e.getTargetException());
489+
if (e.getTargetException() instanceof JsiiError){
490+
throw (JsiiError)(e.getTargetException());
491+
} else if (e.getTargetException() instanceof RuntimeException) {
492+
// can rethrow without wrapping here
493+
throw (RuntimeException)(e.getTargetException());
494+
} else {
495+
// Can't just throw a checked error without wrapping it :(
496+
throw new RuntimeException(e.getTargetException());
497+
}
490498
} catch (IllegalAccessException e) {
491499
throw new JsiiError(e);
492500
} finally {
@@ -502,9 +510,12 @@ private Object invokeMethod(final Object obj, final Method method, final Object.
502510
private void processCallback(final Callback callback) {
503511
try {
504512
JsonNode result = handleCallback(callback);
505-
this.getClient().completeCallback(callback, null, result);
506-
} catch (JsiiError e) {
507-
this.getClient().completeCallback(callback, e.getMessage(), null);
513+
this.getClient().completeCallback(callback, null, null, result);
514+
} catch (Exception e) {
515+
String name = e instanceof JsiiException
516+
? JsiiException.Type.JSII_FAULT.toString()
517+
: JsiiException.Type.RUNTIME_ERROR.toString();
518+
this.getClient().completeCallback(callback, e.getMessage(), name, null);
508519
}
509520
}
510521

‎packages/@jsii/java-runtime/project/src/main/java/software/amazon/jsii/JsiiException.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ public abstract class JsiiException extends RuntimeException {
88

99
static enum Type {
1010
JSII_FAULT("@jsii/kernel.Fault"),
11-
RUNTIME_EXCEPTION("@jsii/kernel.RuntimeException");
11+
RUNTIME_ERROR("@jsii/kernel.RuntimeError");
1212

1313
private final String errorType;
1414

‎packages/@jsii/java-runtime/project/src/main/java/software/amazon/jsii/JsiiRuntime.java‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,7 @@ private JsonNode processErrorResponse(final JsonNode resp) {
123123
errorMessage += "\n" + resp.get("stack").asText();
124124
}
125125

126-
if (errorName.equals(JsiiException.Type.RUNTIME_EXCEPTION.toString())) {
126+
if (errorName.equals(JsiiException.Type.RUNTIME_ERROR.toString())) {
127127
throw new RuntimeException(errorMessage);
128128
}
129129

@@ -146,6 +146,7 @@ private JsonNode processCallbackResponse(final JsonNode resp) {
146146

147147
JsonNode result = null;
148148
String error = null;
149+
String name = null;
149150
try {
150151
result = this.callbackHandler.handleCallback(callback);
151152
} catch (Exception e) {
@@ -154,12 +155,17 @@ private JsonNode processCallbackResponse(final JsonNode resp) {
154155
} else {
155156
error = e.getMessage();
156157
}
158+
159+
name = e instanceof JsiiError
160+
? JsiiException.Type.JSII_FAULT.toString()
161+
: JsiiException.Type.RUNTIME_ERROR.toString();
157162
}
158163

159164
ObjectNode completeResponse = JsonNodeFactory.instance.objectNode();
160165
completeResponse.put("cbid", callback.getCbid());
161166
if (error != null) {
162167
completeResponse.put("err", error);
168+
completeResponse.put("name", name);
163169
}
164170
if (result != null) {
165171
completeResponse.set("result", result);

‎packages/@jsii/kernel/src/api.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -217,6 +217,7 @@ export interface CallbacksResponse {
217217
export interface CompleteRequest {
218218
readonly cbid: string;
219219
readonly err?: string;
220+
readonly name?: string;
220221
readonly result?: any;
221222
}
222223

‎packages/@jsii/kernel/src/kernel.ts‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -470,9 +470,15 @@ export class Kernel {
470470
try {
471471
result = await promise;
472472
this._debug('promise result:', result);
473-
} catch (e) {
473+
} catch (e: any) {
474474
this._debug('promise error:', e);
475-
throw new JsiiFault((e as any).message);
475+
if (e.name === JsiiErrorType.JSII_FAULT) {
476+
throw new JsiiFault(e.message);
477+
}
478+
479+
// default to RuntimeError, since non-kernel errors may not
480+
// have their `name` field defined
481+
throw new RuntimeError(e);
476482
}
477483

478484
return {
@@ -505,7 +511,7 @@ export class Kernel {
505511
}
506512

507513
public complete(req: api.CompleteRequest): api.CompleteResponse {
508-
const { cbid, err, result } = req;
514+
const { cbid, err, result, name } = req;
509515

510516
this._debug('complete', cbid, err, result);
511517

@@ -516,7 +522,11 @@ export class Kernel {
516522

517523
if (err) {
518524
this._debug('completed with error:', err);
519-
cb.fail(new Error(err));
525+
cb.fail(
526+
name === JsiiErrorType.JSII_FAULT
527+
? new JsiiFault(err)
528+
: new RuntimeError(err),
529+
);
520530
} else {
521531
const sandoxResult = this._toSandbox(
522532
result,

‎packages/@jsii/kernel/src/recording.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ export function recordInteraction(kernel: Kernel, inputOutputLogPath: string) {
6060
return ret;
6161
} catch (e: any) {
6262
logOutput({ error: e.message, name: e.name });
63-
if (e.type === JsiiErrorType.RUNTIME_ERROR) {
63+
if (e.name === JsiiErrorType.RUNTIME_ERROR) {
6464
throw new RuntimeError(e.message);
6565
}
6666
throw new JsiiFault(e.message);

‎packages/@jsii/python-runtime/src/jsii/_kernel/providers/process.py‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@
6262
CompleteRequest,
6363
CompleteResponse,
6464
)
65-
from ...errors import JSIIError, JavaScriptError
65+
from ...errors import ErrorType, JSIIError, JavaScriptError
6666

6767

6868
@attr.s(auto_attribs=True, frozen=True, slots=True)
@@ -83,10 +83,11 @@ class _OkayResponse:
8383

8484

8585
@attr.s(auto_attribs=True, frozen=True, slots=True)
86-
class _ErrorRespose:
86+
class _ErrorResponse:
8787

8888
error: str
8989
stack: str
90+
name: str
9091

9192

9293
@attr.s(auto_attribs=True, frozen=True, slots=True)
@@ -101,7 +102,7 @@ class _CompleteRequest:
101102
complete: CompleteRequest
102103

103104

104-
_ProcessResponse = Union[_OkayResponse, _ErrorRespose, _CallbackResponse]
105+
_ProcessResponse = Union[_OkayResponse, _ErrorResponse, _CallbackResponse]
105106

106107

107108
def _with_api_key(api_name, asdict):
@@ -326,6 +327,8 @@ def send(
326327
elif isinstance(resp, _CallbackResponse):
327328
return resp.callback
328329
else:
330+
if resp.name == ErrorType.RUNTIME_ERROR.value:
331+
raise RuntimeError(resp.error) from JavaScriptError(resp.stack)
329332
raise JSIIError(resp.error) from JavaScriptError(resp.stack)
330333

331334

0 commit comments

Comments
 (0)