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

Add RMQSession#addShutdownListenerToChannel method #133

Closed
wants to merge 1 commit into from

Conversation

edmundham
Copy link

Hi RabbitMQ JMS Team,

I'd like to add RMQSession#addShutdownListenerToChannel method.
This will allow users to add their own customized ShutdownListener which could be used for recovering closed RMQSession.

Our use case specifically is when Session is closed due to MessageListener#onMessage taking too long (timeout), there is no way to catch exception and recreate Session because it is handled internally in different thread. By using RMQSession#addShutdownListenerToChannel method, we can detect session closure and recover if necessary.

Please let me know what you guys think :)

Best,
Edmund

@acogoluegnes
Copy link
Contributor

I don't see any problems adding the method if it can help you. The thing is it will require an explicit cast, which kinda ruins the whole idea of JMS with regards to vendor independence. This is tolerable in case of emergency though.

I think the problem here is the fact the session is closed because of the timeout. I'm not the original author of the library, so I don't know why such a radical decision was taken (the timeout was even fixed to 2 seconds at first).

We already had a similar discussion on the mailing list. We concluded that we could introduce an option to nack and requeue messages that time out. I think this is a more reasonable solution that doing acrobatics to recover the session. WDYT?

/cc @sdurrenmatt

@edmundham
Copy link
Author

@acogoluegnes Thanks for the comment. Yes I agree that this would ruin the whole idea of JMS but in our case specifically we have an extra abstract layer so hopefully that is the case for the future consumer of the method too.

I've read the discussion on the mailing list. However I still have one concern. In the case of messages being timed out, it does make sense to nack and requeue, but would the channel (that is used for message that is timed out) still be open? I thought the workflow is that channel is closed regardless of messages being acked or nacked, because message has been timed out anyway. Please correct me if I'm misunderstanding here, of course you are the expert here :)

@acogoluegnes
Copy link
Contributor

Sorry for the late reply, I missed your answer.

The AMQP channel does not have to be closed in this case. There is no timeout in AMQP, the timeout here is introduced by JMS, so there's no definitive reason to close the AMQP channel in this case.

A message can be rejected and re-queued without the need to close the channel with AMQP.

@edmundham
Copy link
Author

Thanks team, I know RMQ JMS library isn't frequently being released but is there any rough timeline of when would this be released?

@acogoluegnes
Copy link
Contributor

@edmundham I'm still thinking about adding an option to requeue messages in case of timeout. WDYT?

@edmundham
Copy link
Author

@acogoluegnes In my personal opinion, that should remain as developer's option. Currently using the RMQConnectionFactory#setRequeueOnMessageListenerException, I can requeue messages in case of timeout.

The best I can see is to have

RMQConnectionFactory#setRequeueOnMessageListenerException
RMQConnectionFactory#setRequeueOnTimeoutException

so that developers can differentiate the behaviour on MessageListenerException and TimeoutException, but this is an actual feature request and I don't think this is entirely necessary.

@acogoluegnes
Copy link
Contributor

@edmundham I added an extra requeueOnTimeout flag. You can have a look at the snapshot: https://oss.sonatype.org/content/repositories/snapshots/com/rabbitmq/jms/rabbitmq-jms/2.3.0-SNAPSHOT/.

If this is fine, I'll release an RC.

@edmundham
Copy link
Author

@acogoluegnes That sounds great, thanks for your change. Yes, we can move forward to have an RC.
Appreciate your help :)

@acogoluegnes
Copy link
Contributor

@edmundham 2.3.0.RC1 is available.

@edmundham
Copy link
Author

Thanks :)

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

Successfully merging this pull request may close these issues.

None yet

3 participants