mateodelnorte / servicebus

Simple service bus for sending events between processes using amqp.
MIT License
420 stars 66 forks source link

How to handle malformed JSON on message payload? #77

Open keizohirata opened 8 years ago

keizohirata commented 8 years ago

I was running some tests sending the messages directly from RabbitMQ Manager. Then I sent a malformed JSON and my application stopped with the following exception:

events.js:160
      throw er; // Unhandled 'error' event
      ^

 in JSON at position 31 token
    at Object.parse (native)
    at Object.deserialize (/opt/mensmarket/mm-catalog-service/node_modules/servicebus/bus/formatters/json.js:4:20)
    at listenChannel.consume.noAck (/opt/mensmarket/mm-catalog-service/node_modules/servicebus/bus/rabbitmq/queue.js:93:41)
    at Channel.BaseChannel.dispatchMessage (/opt/mensmarket/mm-catalog-service/node_modules/amqplib/lib/channel.js:466:12)
    at Channel.BaseChannel.handleDelivery (/opt/mensmarket/mm-catalog-service/node_modules/amqplib/lib/channel.js:475:15)
    at emitOne (events.js:96:13)
    at Channel.emit (events.js:188:7)
    at /opt/mensmarket/mm-catalog-service/node_modules/amqplib/lib/channel.js:263:10
    at Channel.content [as handleMessage] (/opt/mensmarket/mm-catalog-service/node_modules/amqplib/lib/channel.js:316:9)
    at Channel.C.acceptMessageFrame (/opt/mensmarket/mm-catalog-service/node_modules/amqplib/lib/channel.js:231:31)

I thought that this kind of issue should be solved by the servicebus-retry package. So after N attempts to process the message, I thought that the message could be put on the '.error' queue. But I was wrong and the application just stopped.

Am I doing something wrong on how to handle this situation?

mateodelnorte commented 8 years ago

Since servicebus-retry is a middleware, it doesn't yet have access to the message content at this point, because it hasn't been parsed and turned into json.

I think, long term, the way to fix this is to no longer have formatters. You would just have middleware, and everyone would likely just use a json middleware first and the first thing it would do is JSON.parse on the incoming message content. This would be a big breaking change going forward, so it's something I'd do as part of a major release.

What you might be able do to alleviate this is 1) set your formatter (used here https://github.com/mateodelnorte/servicebus/blob/670ebfc2d7fc095a82e60d14e2025f15747dfeb8/bus/rabbitmq/queue.js#L93) as a module that implements this interface https://github.com/mateodelnorte/servicebus/blob/master/bus/formatters/json.js and simply returns content in both scenarios. 2) create a json middleware that does stringify on outboundMessage and parse on inboundMessage.

It's possible, however that you may need two middleware's however - one at the top of your stack for sending and one at the bottom of your stack for inbound.

mateodelnorte commented 7 years ago

Hi @keizohirata. I've created a PR at #91. Can you test and see if this gives you the behavior you'd prefer?

aangelidis commented 7 years ago

Can you not just make it a "string" and let the consumer handle the format as needed?

mateodelnorte commented 7 years ago

The problem is the rippling effect up the middleware chain it would require to get the retry behavior expected. upstream middleware, including servicebus-retry, expect the incoming message to be an object, in order to append functions on it which it then uses to ack or reject. You could change the formatter to be a middleware, but it would need to be higher in the stack than retry. There are some options for doing this refactoring (allow middleware to pass errors forward to the end of the pipeline, make retry a sort of plugin instead of middleware) but all would introduce a breaking change.

alexkazantsev commented 7 years ago

@mateodelnorte , there are any reason why #91 isn't merged yet?

mateodelnorte commented 7 years ago

yeah, @alexkazantsev, it doesn't really handle the issue correctly. What you really want is to catch the json parse error in the middle of the middleware chain, after retry() has seen the message, so you can call reject() on it and have it try again. As it stands, the parsing is before the middleware pipeline (because all the middleware rely on the message being an object).

so, it would really take a larger re-architecting of the middleware pipeline and probably a reworking of the retry middleware. happy to do it, but it's going to be a larger effort and other things currently have priority. if it's something you'd like to take a swack at, I'd be happy to talk all about it and lead you through on nycnode.com/slack or on here.