Skip to content
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

How to handle malformed JSON on message payload? #77

Open
keizohirata opened this issue Oct 12, 2016 · 6 comments
Open

How to handle malformed JSON on message payload? #77

keizohirata opened this issue Oct 12, 2016 · 6 comments

Comments

@keizohirata
Copy link

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
Copy link
Owner

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

message.content = options.formatter.deserialize(message.content);
) 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
Copy link
Owner

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

@aangelidis
Copy link

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

@mateodelnorte
Copy link
Owner

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
Copy link

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

alexkazantsev pushed a commit to S-PRO/servicebus that referenced this issue May 11, 2017
@mateodelnorte
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants