No error when exceeding message limit

[lperrin]

The Pusher limits messages to 10Kb. This is perfectly fine, but the driver silently fails in this case. Having messages randomly disappear was quite puzzling for us :)

[leggetter]

Did the callback not occur with an error status code?

https://github.com/pusher/pusher-node-server/blob/master/lib/pusher.js#L52

There's a test in place to check this:
https://github.com/pusher/pusher-node-server/blob/master/tests/acceptance-tests/trigger-test.js#L36

Please let me know if this doesn't work as expected.

[lperrin]

When I noticed that some messages were dropped, I added this code directly in production (the documentation doesn't mention the callback anymore and I wasn't sure what was causing the issue):

  pusher.trigger(privateRoom, eventName, message, null, function (err) {
    if (err)
      console.error('pusher error on', eventName, err);
  });

Without seeing anything in the console. After thinking about it, it seems that err is only set when the POST request fails (not the case here), not when the trigger is unsuccessful.

[leggetter]

it seems that err is only set when the POST request fails (not the case here), not when the trigger is unsuccessful.

That would make sense since a 413 is not an error code.

Couple of options here:

1. README needs updating to show the callback and the 413 needs to be highlighted.
2. The library needs to check for a 413 and set the err object accordingly

@mloughran @mdpye What do you guys think?

[lperrin]

IMO, updating the README to warn about the 10Kb limit and the format of the answer is enough. There's no need to introduce a breaking change for that.

[mdpye]

I'm not familiar with the code, but HTTP 413 is absolutely an error and the POST request has failed. We should deal with that in the same way we might a 400 or 403

[leggetter]

The request hasn't failed hasn't errored, which may be what the err is for?

I tried adding a test:

'when triggering a string over 10kB': {
    topic: function() {
        var buf = new Buffer(1024*11);
        var str = buf.toString();

        pusher.trigger( 'test_channel', 'my_event', str, null, this.callback );
    },

    // 'the REST API call will return 413': function(err, req, res) {
   //    assert.equal( res.statusCode, 413 );
   //  },

    'the response is an error': function(err, req, res) {
      assert.isObject( err );
    } 
  }

And the err object isn't populated. This is an object directly from the [http.ClientRequest](http://nodejs.org/api/http.html#http_class_http_clientrequest). See callback info in the request module docs:
https://github.com/mikeal/request#requestoptions-callback

Could try testing adding test which would result in a 400 from the API and see if the error object is populated.

[lperrin]

The reason why this was puzzling to me was that when a call a module, I expect it to pass an error to my callback if my request is unsuccessful.

That the error comes from a hanged TCP socket or an HTTP status is an implementation detail to me. The current behavior should be documented because it's not most common node.js modules behave.

From the back of my head, here are modules that use an internal HTTP request, yet pass errors to callback when they get 4XX statuses: felix-couchdb, node-mongodb, hipchat, twitter and oauth.

Now, there's nothing wrong with the current behavior, but having the documentation stress that the callback passes a HTTP response would have saved me a couple of hours of bug hunting.

[mdpye]

In general, I agree with @lperrin. The library wraps an API, the HTTP transport of which shouldn't leak into the distinction between transport and application errors. Ideally, all non-200 status codes should be translated to an error in the callback, because the API semantics specify that they are errors. However, that is a large change to the current behaviour and I'm not going to dive into it now.

In the mean time, how about f59dd1e