Redactor Error Handler / PostgreSQL Error on 8000+ characters message

JJSagan

Hi Devs,
Got the below error when I was sending a message with Redactor,
Kindly note the DeprecationWarning.
Any idea?

(node:3282) UnhandledPromiseRejectionWarning: error: payload string too long
    at Connection.parseE (nodebb/node_modules/pg/lib/connection.js:553:11)
    at Connection.parseMessage (nodebb/node_modules/pg/lib/connection.js:378:19)
    at Socket.<anonymous> (nodebb/node_modules/pg/lib/connection.js:119:22)
    at Socket.emit (events.js:182:13)
    at Socket.EventEmitter.emit (domain.js:442:20)
    at addChunk (_stream_readable.js:283:12)
    at readableAddChunk (_stream_readable.js:264:11)
    at Socket.Readable.push (_stream_readable.js:219:10)
    at TCP.onStreamRead [as _originalOnread] (internal/stream_base_commons.js:94:17)
    at TCP.onread (nodebb/node_modules/async-listener/glue.js:188:31)
(node:3282) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
(node:3282) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

JJSagan

Hi Devs,

The above issue repeated today, when a user opened Redactor, uploaded an image and tried to post the message.
Redactor failed and the only way to recover it was to close it and re-open.

With some debugging I was able to identify two issues:

Postgress driver - looking at postgress logs, note the following:

2018-12-26 13:07:28 UTC:172-15-10-85.lightspeed.snjsca.sbcglobal.net(33758):[9559]:ERROR: payload string too long
2018-12-26 13:07:28 UTC:172-15-10-85.lightspeed.snjsca.sbcglobal.net(33758):[9559]:STATEMENT: SELECT pg_notify('socketio', $1::TEXT)
2018-12-26 13:07:28 UTC:172-15-10-85.lightspeed.snjsca.sbcglobal.net(33754):[9557]:ERROR: payload string too long
2018-12-26 13:07:28 UTC:172-15-10-85.lightspeed.snjsca.sbcglobal.net(33754):[9557]:STATEMENT: SELECT pg_notify('socketio', $1::TEXT)

SELECT pg_notify('socketio', $1::TEXT) is likely sourced from node_modules/socket.io-adapter-postgres/index.js, line 118.

It seems that the driver may have an error related to session hash strings. The postgress has a hard stop at 8000 characters and it might be that the hash string is too big. I don't know much about postgres so this is as far as I can debug.

It seems however that the entire code was re-written in the pull request by @Ben-Lubar here:
https://github.com/NodeBB/NodeBB/pull/6745

Is it possible to merge the code revision into the main branch?

Redactor:
Currently, upon file load failure, redactor fails and does not recover.
That's easy to reproduce - just add a hook with

{ "hook": "filter:uploadFile",          "method": "myUploadFile", "priority": 6 }

In myUploadFile just return an error for example:

return calback(Error('myError'));

Redactor fails in line 10697 here:

                    xhr.send(formData);

Since errors are not handled , JS stops dead on the spot. Furthermore, the user does not get any indication of the error, although the xhr packet does contain an error string. With that, the only way to recover is to close the editor and re-open it.

These two issues are independent. The fix in Redactor is simple I believe (handling the error, toasting a message). As for PG, I hope the new code incorporates a fix.

Kindly let me know should you need further information,

Longterm, I hope quill will replace Redactor. There's not a lot of things left to do to make https://github.com/NodeBB/nodebb-plugin-composer-quill/ production ready. But I totally realize @Julian is extremely busy.

JJ.

JJSagan

@JJSagan said in Redactor Error Handler / PostgreSQL Driver potential issue:

socket.io-adapter-postgres

More on (1):
The issue (see first message) is 100% reproducible. Just type in a large message well over 8000 characters.

As for the source:
SELECT pg_notify('socketio', $1::TEXT) is sourced from node_modules/socket.io-adapter-postgres/index.js, line 118.

It is triggered when the payload is too big.

I see that @Ben-Lubar has a newer version of socket.io-adapter-postgres.

The new code (see: https://github.com/BenLubar/socket.io-adapter-postgres/blob/master/index.js) breaks the payload to chunks (to overcome PG 8000 characters limit):

this._publish = function(channel, payload) {
      if (payload.length > 8000) {
        // https://jsperf.com/string-split-by-length/3
        var chunks = payload.match(new RegExp('[\\s\\S]{1,5000}', 'g'));
        var payloadid = uid2(6);
        for (var i = 0; i < chunks.length; i++) {
          var header = _.padEnd('_chunk_' + payloadid + '_' + i + '_' + chunks.length, 50);
          var chunk = header + chunks[i];
          pub.query('SELECT pg_notify(\'socketio\', $1::TEXT)', [JSON.stringify([channel, chunk])]);
        }
      } else {
        pub.query('SELECT pg_notify(\'socketio\', $1::TEXT)', [JSON.stringify([channel, payload])]);
      }
    };

Does anybody know if it is safe to use the new code? I would hate to have it demolish my database
Thanks!
JJ.

JJSagan

Hi Devs,

Since the errors kept hitting, I took a chance and updated node_modules/socket.io-adapter-postgres/index.js with the code from: https://github.com/BenLubar/socket.io-adapter-postgres/blob/master/index.js

Thank god It worked: no more issues on long posts.

You may want to update the same, so other PostgreSQL users may benefit. Without this revision, the DB may become faulty over time.

Still, issue (2) remains - but resolving it is quite simple: just add a simple toast so the user will know the upload failed (and for what reason). This will also make the plugin robust (i.e., closing/re-opening the editor won't be necessary).

Thanks,
JJ.

JJSagan

Reported the redactor related issue in github:
https://github.com/NodeBB-Community/nodebb-plugin-composer-redactor/issues/74

JJSagan

For anyone following this thread:, the error handler in Redactor will not be fixed, as the issue lies in the vendor code that NodeBB dev. team is not licensed to edit. See comments here:
https://github.com/NodeBB-Community/nodebb-plugin-composer-redactor/issues/74

Redactor users are advised to wait for Quill (https://github.com/NodeBB/nodebb-plugin-composer-quill).

kfirba

@JJSagan This issue isn't only with Redactor.

You can easily reproduce that issue by writing custom CSS/LESS in your dashboard. As it turns out, Postgress cannot emit messages larger than 8000 bytes.

There should be some mechanism that prevents from a notification to be broadcast if the payload is bigger than 8k bytes. Alternatively, truncate the notification OR send the notification in batches (which will require some front-end adaptations).

Also, the switches that enable or disable the "live reloading" or "live updating" aren't disabling the notification itself, they just don't LISTEN to the notification so turning those off does not resolve the error.

This error is all over in version 1.10.2. In version 1.11.1 I didn't get an error when adding custom CSS/LESS so maybe something changed?

cc @julianlam

julian

Thanks for reporting, I've raised it internally with @Ben-Lubar who is the one who helped us get Postgres integration into NodeBB.

JJSagan

@kfirba try updating socket.io-adapter-postgres (replace index.js with https://github.com/BenLubar/socket.io-adapter-postgres/blob/master/index.js). It should resolve the issue you have encountered in earlier releases. I reported this in github if you wish follow:
https://github.com/NodeBB/NodeBB/issues/7215