New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#2650 Initialise detailMessage for PyException #97

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@jamesmudd
Contributor

jamesmudd commented Dec 8, 2017

No description provided.

@jeff5

This comment has been minimized.

Show comment
Hide comment
@jeff5

jeff5 Jan 2, 2018

Member

I think this approach to exceptions, deferring the creation of a message, is deliberate.

Exceptions in Python are a normal means of control transfer, whereas in Java they're expicitly not. (I could find the refs, I think.) Once we come to print the thing out, of course, then we know it's not just a transfer of control, but I think we're putting it off until then.

Your use case is good, however. My instinct is that the exception ought not to escape an embedded interpreter without a proper message.

We get to do that for the JSR223 engine here, by wrapping:

private static ScriptException scriptException(PyException pye) {

But PythonInterpreter gives you the actual PyException, being a thin veneer over PySytemState and the built-in eval.

Would it work to define PyException.getMessage() so it yields the text you hope for?

Member

jeff5 commented Jan 2, 2018

I think this approach to exceptions, deferring the creation of a message, is deliberate.

Exceptions in Python are a normal means of control transfer, whereas in Java they're expicitly not. (I could find the refs, I think.) Once we come to print the thing out, of course, then we know it's not just a transfer of control, but I think we're putting it off until then.

Your use case is good, however. My instinct is that the exception ought not to escape an embedded interpreter without a proper message.

We get to do that for the JSR223 engine here, by wrapping:

private static ScriptException scriptException(PyException pye) {

But PythonInterpreter gives you the actual PyException, being a thin veneer over PySytemState and the built-in eval.

Would it work to define PyException.getMessage() so it yields the text you hope for?

@jamesmudd

This comment has been minimized.

Show comment
Hide comment
@jamesmudd

jamesmudd Jan 3, 2018

Contributor

@jeff5 Thanks for the comments. Yes overriding PyException.getMessage() would also work, and would probably be preferable in this case for the performance reason mentioned.

I implemented it in a Java way i.e. exceptions shouldn't be used for flow control, therefore if you are creating one you should make it contain all the information it needs and ideally make it immutable.

My only question would be is making the detail message noticeable when you consider that your about to create the Java stack trace anyway? I would have guessed that took much longer? But for now I will change this to lazily build the message.

Contributor

jamesmudd commented Jan 3, 2018

@jeff5 Thanks for the comments. Yes overriding PyException.getMessage() would also work, and would probably be preferable in this case for the performance reason mentioned.

I implemented it in a Java way i.e. exceptions shouldn't be used for flow control, therefore if you are creating one you should make it contain all the information it needs and ideally make it immutable.

My only question would be is making the detail message noticeable when you consider that your about to create the Java stack trace anyway? I would have guessed that took much longer? But for now I will change this to lazily build the message.

@jeff5

This comment has been minimized.

Show comment
Hide comment
@jeff5

jeff5 Jan 3, 2018

Member

Yes, it's more natural for Java, but the (C)Python way is influenced by good old errno. We simply keep all the broken pieces in the thread state, and I think there's no real exception object until it's caught. The PyException isn't a PyObject.

The Java stack trace might be no more than a pointer to the last frame at this stage. The Python one is a linked list of names, and no-one does any further text formatting until asked:

public PyTraceback(PyTraceback next, PyFrame frame) {
super(TYPE);
tb_next = next;
tb_frame = frame;
tb_lineno = frame.f_code.getline(frame);
}

The most wasteful part may be the way we seem to re-throw the (Java) exception to work our way our of each (Python) frame, but I guess in that case you probably are heading for a stack trace not a control transfer.

Member

jeff5 commented Jan 3, 2018

Yes, it's more natural for Java, but the (C)Python way is influenced by good old errno. We simply keep all the broken pieces in the thread state, and I think there's no real exception object until it's caught. The PyException isn't a PyObject.

The Java stack trace might be no more than a pointer to the last frame at this stage. The Python one is a linked list of names, and no-one does any further text formatting until asked:

public PyTraceback(PyTraceback next, PyFrame frame) {
super(TYPE);
tb_next = next;
tb_frame = frame;
tb_lineno = frame.f_code.getline(frame);
}

The most wasteful part may be the way we seem to re-throw the (Java) exception to work our way our of each (Python) frame, but I guess in that case you probably are heading for a stack trace not a control transfer.

#2650 Override getMessage in PyException
This allows PyException's leaving the interpreter to be handled better
by Java logging frameworks.
@jamesmudd

This comment has been minimized.

Show comment
Hide comment
@jamesmudd

jamesmudd Jan 3, 2018

Contributor

I have updated this PR to override getMessage instead of initializing the Throwable detailMessage field.

Contributor

jamesmudd commented Jan 3, 2018

I have updated this PR to override getMessage instead of initializing the Throwable detailMessage field.

@jeff5

This comment has been minimized.

Show comment
Hide comment
@jeff5

jeff5 Jan 3, 2018

Member

Thanks James! Now committed at: https://hg.python.org/jython/rev/d74f8c2cd56f and d221444

Member

jeff5 commented Jan 3, 2018

Thanks James! Now committed at: https://hg.python.org/jython/rev/d74f8c2cd56f and d221444

@jeff5 jeff5 closed this Jan 3, 2018

@jamesmudd jamesmudd deleted the jamesmudd:2650 branch Jul 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment