eclipse / paho.mqtt.javascript

paho.mqtt.javascript
Other
1.15k stars 467 forks source link

errors in user code are swallowed, along with stack traces #25

Closed jpwsutton closed 8 years ago

jpwsutton commented 8 years ago

migrated from Bugzilla #440126 status RESOLVED severity normal in component MQTT-JS for 1.1 Reported in version 1.0 on platform PC Assigned to: Tang Zi Han

Original attachment names and IDs:

On 2014-07-22 10:56:21 -0400, Karl Palsson wrote:

If a user supplied onMessage handler produces an error, you simply get an "AMQJS0005E Internal error" with error code 5 in the onConnectionLost handler.

However, if you set a breakpoint at Paho.MQTT.ClientImpl._handleMessage (mqttws31.js:1343) You can see that error.message has the real cause, and that the code clearly attempts to preserve it, but either the usage of format is wrong, or format itself is wrong. Either way, you don't get the error.message preserved.

Further, presumably because of the way the "scope" function is created, you lose the stack trace taking you back to the original user supplied function. This seems like a backwards step, the stack trace below is all that chromium developer tools will show me, when I stop where I can see my error, but the "anonymous function" is the "scope" function, not my own function, so I have to read the error, and then try and infer which method of my own might have caused it.

Paho.MQTT.ClientImpl._handleMessage (mqttws31.js:1343)
Paho.MQTT.ClientImpl._on_socket_message (mqttws31.js:1152) (anonymous function) (mqttws31.js:157)

On 2014-07-22 11:01:35 -0400, Karl Palsson wrote:

Created attachment 245270 demonstrates failure to preserve errors or stacktraces

On 2014-07-22 11:07:01 -0400, Karl Palsson wrote:

Example of what error.stack looks like in the _handleMessage exception handler, showing that it is useful, before the js library swallows it:

error.stack "ReferenceError: wop is not defined at onMessageArrived (http://192.168.255.38/cgi-bin/luci/;stok=9ea51323a52992a6ae727f50301d7492/home/hoho:50:16) at Paho.MQTT.ClientImpl._receiveMessage (http://192.168.255.38/resources/paho/mqttws31.js:1402:9) at Paho.MQTT.ClientImpl._receivePublish (http://192.168.255.38/resources/paho/mqttws31.js:1377:10) at Paho.MQTT.ClientImpl._handleMessage (http://192.168.255.38/resources/paho/mqttws31.js:1251:10) at Paho.MQTT.ClientImpl._on_socket_message (http://192.168.255.38/resources/paho/mqttws31.js:1152:12) at WebSocket.Paho.MQTT.scope (http://192.168.255.38/resources/paho/mqttws31.js:157:13)"

On 2014-08-10 22:44:10 -0400, Tang Zi Han wrote:

I can reproduce the scenario and try to fix it .

On 2014-08-11 03:52:14 -0400, Tang Zi Han wrote:

The config of Error text do not leave a placeholder to format error message and stack trace.

Fixed and Close.

Karl's description and demonstration is very helpful.

On 2014-08-11 04:46:32 -0400, Tang Zi Han wrote:

Change the Error variable to INTERNAL_ERROR: {code:5, text:"AMQJS0005E Internal error. Error Message: {0}, Stack trace: {1}"},

and add error message and stack trace to related callback. this._disconnected(ERROR.INTERNAL_ERROR.code , format(ERROR.INTERNAL_ERROR, [error.message,error.stack.toString()]));

push the fix to develop branch

On 2014-08-11 13:47:23 -0400, Karl Palsson wrote:

This is a substantial improvement, but it could still be better. Is there any actual reason not to just let it bubble up as it is? In chromium, the string representation is parsed, and you can click on the elements of the stack trace nicely. Firefox just shows it as raw text.

On 2014-08-13 23:07:33 -0400, Tang Zi Han wrote:

(In reply to Karl Palsson from comment # 6)

This is a substantial improvement, but it could still be better. Is there any actual reason not to just let it bubble up as it is? In chromium, the string representation is parsed, and you can click on the elements of the stack trace nicely. Firefox just shows it as raw text.

The actual reason is that the try-catch block in ClientImpl.prototype._handleMessage. If some error occures in the callback function which is called based on the MESSAGE_TYPE , it will go to the catch block to deal with the error or exception. Based on the currently design, the easiest and best way is to bubble up the error to notify developer.