search

Stormpass / node-red-contrib-amqp

Amqp nodes for node-red
https://flows.nodered.org/node/@stormpass/node-red-contrib-amqp
2 stars 4 forks source link

Node-RED disconnected from RabbitMQ and not reconnecting #5

Closed corentin-pasquier closed 3 months ago

corentin-pasquier commented 4 months ago

Hello,

I have a problem with how the connection error is (not) implemented: https://github.com/Stormpass/node-red-contrib-amqp/blob/main/src/Amqp.ts#L74 When there is a connection error, Node-RED disconnect from RabbitMQ and does not attempt to reconnect. I have to redeploy the node to recreate a connection.

Would it be possible to force a reconnection when there is a connecion error ? Or maybe there is another way (that I don't know about) to handle it in Node-RED ?

Thank you.

Stormpass commented 4 months ago

In my opinion, you should use node-red's catch node to catch the error events from the amqp node and then decide that whether to reconnect or not. But currently the error thrown by Amqp node can't be caught by node-red, I will fix it.

corentin-pasquier commented 4 months ago

Hello, I don't know how I can start a connection after catching an error. However I think I found a bug with the manual ack node (I could reproduce it with the example provided). The problem is thrown when I send several messages through this node. When this happens, the connection is closed forever.

Here is a picture of the error, sorry I can't paste it properly: image

Edit: The error i mentioned happens when RabbitMQ close the connection, for example when you try to ack the same message twice (I know this shouldn't happen, but this is an easy way to reproduce the problem).

image

After this, RabbitMQ close the connection but in Node-RED the node thinks it's still connected and do nothing.

corentin-pasquier commented 4 months ago

Hello @Stormpass, I saw you made some changes last week. I forked your project to try a few changes as well: https://github.com/Stormpass/node-red-contrib-amqp/compare/main...corentin-pasquier:node-red-contrib-amqp:main

The node now tries to reconnect to the server when the connection closes. I also added a maxAttempts parameter in order to control how many times the node tries to connect when the server is down.

Maybe you can take a look ? I can also create a PR if you want. I don't want to keep my fork up, I don't think it's a good idea to have several projects doing the same thing.

Stormpass commented 4 months ago

Sorry for being busy in recent times. Didn't have enough time to finish that thing we discussed above, thanks for your work and I'm glad you submitted a PR. But I don't see how maxAttempts is controlled, are you in the process of patching that up. To clarify, the library is not original to me. I forked it from @meowwolf/node-red-contrib-amqp because I was in a hurry to fix a couple issues.

Stormpass commented 4 months ago

I don't want to keep my fork up, I don't think it's a good idea to have several projects doing the same thing.

I agree. I've just invited you to become collaborators

corentin-pasquier commented 4 months ago

MaxAttempts can be set in the node configuration. Maybe I could have put this configuration in the broker definition but since every node has its own connection, I think it's better to keep it as a node coniguration. image I'll create several PR, one for the maxAttempts and one for the reconnect part. Feel free to review and comment my changes. :)

corentin-pasquier commented 3 months ago

Hello @Stormpass I pushed two new feature branches. I hope my code id ok. 🤞

Stormpass commented 3 months ago

Hello @Stormpass I pushed two new feature branches. I hope my code id ok. 🤞

Greet ! feature merged

Stormpass commented 3 months ago

Suddenly，I realized that “MaxAttempts “ maybe not a good idea it is share by close and error event， and the old version will reconnect forever when close event fired. it can cause confuse. separate close and error will make code a big change. so i think we could replace “MaxAttempts “ to "reconnectOnError", let user decide to use it or not; if user want to limit MaxAttempts , just catch the error event and reconnect by manualy should be enough.

corentin-pasquier commented 3 months ago

Fine for me, but I'm still wondering how you can catch the error and manualy reconnect ? Thanks for the merge

zazin commented 1 month ago

I also experienced the same issue, which is an error when restarting the application. I finally found a solution to that problem, so I replaced that module with this one.

https://flows.nodered.org/node/@corentin-pasquier/node-red-contrib-amqp