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

Starting DirectMessageListenerContainer does not addConsumerToRestart when consumer failed to start #1296

Closed
davsclaus opened this issue Jan 12, 2021 · 4 comments · Fixed by #1297

Comments

@davsclaus
Copy link

Affects Version(s): 2.3.2

Bug report

I am using a DirectMessageListenerContainer to consume from RabbitMQ via spring-rabbit.

When I start the listener container via its start method, and the consumer fail to start due to for example queue does not exists. The spring-rabbit logs at DEBUG level

Queue not present, scheduling consumer SimpleConsumer [queue=myqueue, consumerTag=null identity=68fe1f37] for restart`

But unfortunately the consumer is not added to be restarted as the error happens during startup, and the started flag is not yet set to true, as this happens AFTER the consumer is attempted to be started.

See the 3 screenshots from source code debugging.

@davsclaus
Copy link
Author

rabbit-start-3

rabbit-start-2

rabbit-start-1

@davsclaus
Copy link
Author

Maybe add starting flag to the listener

boolean starting = false

And then set

DirectMessageListenerContainer.this.starting = true; in the top of the startConsumers method, so we can check for both starting and started flag in the addConsumerToRestart method

	private void addConsumerToRestart(SimpleConsumer consumer) {
		if (this.starting || this.started) {
			this.consumersToRestart.add(consumer);
			if (this.logger.isTraceEnabled()) {
				this.logger.trace("Consumers to restart now: " + this.consumersToRestart);
			}
		}
	}

@artembilan
Copy link
Member

Thank you for reporting this, Claus!
This is a good catch: how we have missed it with our tests...

With a quick look into the code I would say that we don't need to check that this.started in the addConsumerToRestart() and add consumer into that list unconditionally.

Then we need just to adjust a task in the startMonitor(long idleEventInterval, final Map<String, Queue> namesToQueues) method to not clean this.consumersToRestart if we are not started yet.

WDYT?

/CC @garyrussell

@garyrussell
Copy link
Contributor

@davsclaus Thanks for the detailed analysis.

Caused by #642 so it's a little more involved.

garyrussell added a commit to garyrussell/spring-amqp that referenced this issue Jan 12, 2021
Resolves spring-projects#1296

- Add `MissingQueueEvent`
- Fix detection of a deleted queue in recovery - previously incorrectly used the
  absense of the queue in `consumersByQueue`, which can be empty if missing during
  start
- Add an index to `SimpleConsumer`
- When adjusting consumer counts, look for gaps in the index sequence because reducing
  the consumer count can remove any idle consumer.
- Change consumers to restart to a `Set` to avoid OOM when no broker
  (see spring-projects#642)
- Unconditionally add consumers to `consumersToRestart`

**cherry-pick to 2.2.x, 2.1.x**
garyrussell added a commit to garyrussell/spring-amqp that referenced this issue Jan 12, 2021
Resolves spring-projects#1296

- Add `MissingQueueEvent`
- Fix detection of a deleted queue in recovery - previously incorrectly used the
  absense of the queue in `consumersByQueue`, which can be empty if missing during
  start
- Add an index to `SimpleConsumer`
- When adjusting consumer counts, look for gaps in the index sequence because reducing
  the consumer count can remove any idle consumer.
- Change consumers to restart to a `Set` to avoid OOM when no broker
  (see spring-projects#642)
- Unconditionally add consumers to `consumersToRestart`

**cherry-pick to 2.2.x, 2.1.x**
artembilan pushed a commit that referenced this issue Jan 12, 2021
Resolves #1296

- Add `MissingQueueEvent`
- Fix detection of a deleted queue in recovery - previously incorrectly used the
  absense of the queue in `consumersByQueue`, which can be empty if missing during
  start
- Add an index to `SimpleConsumer`
- When adjusting consumer counts, look for gaps in the index sequence because reducing
  the consumer count can remove any idle consumer.
- Change consumers to restart to a `Set` to avoid OOM when no broker
  (see #642)
- Unconditionally add consumers to `consumersToRestart`

**cherry-pick to 2.2.x, 2.1.x**
artembilan pushed a commit that referenced this issue Jan 12, 2021
Resolves #1296

- Add `MissingQueueEvent`
- Fix detection of a deleted queue in recovery - previously incorrectly used the
  absense of the queue in `consumersByQueue`, which can be empty if missing during
  start
- Add an index to `SimpleConsumer`
- When adjusting consumer counts, look for gaps in the index sequence because reducing
  the consumer count can remove any idle consumer.
- Change consumers to restart to a `Set` to avoid OOM when no broker
  (see #642)
- Unconditionally add consumers to `consumersToRestart`

**cherry-pick to 2.2.x, 2.1.x**

# Conflicts:
#	spring-rabbit/src/main/java/org/springframework/amqp/rabbit/listener/BlockingQueueConsumer.java
#	spring-rabbit/src/test/java/org/springframework/amqp/rabbit/listener/DirectMessageListenerContainerIntegrationTests.java
artembilan pushed a commit that referenced this issue Jan 12, 2021
Resolves #1296

- Add `MissingQueueEvent`
- Fix detection of a deleted queue in recovery - previously incorrectly used the
  absense of the queue in `consumersByQueue`, which can be empty if missing during
  start
- Add an index to `SimpleConsumer`
- When adjusting consumer counts, look for gaps in the index sequence because reducing
  the consumer count can remove any idle consumer.
- Change consumers to restart to a `Set` to avoid OOM when no broker
  (see #642)
- Unconditionally add consumers to `consumersToRestart`

**cherry-pick to 2.2.x, 2.1.x**

# Conflicts:
#	spring-rabbit/src/main/java/org/springframework/amqp/rabbit/listener/BlockingQueueConsumer.java
#	spring-rabbit/src/test/java/org/springframework/amqp/rabbit/listener/DirectMessageListenerContainerIntegrationTests.java

# Conflicts:
#	spring-rabbit/src/main/java/org/springframework/amqp/rabbit/listener/AbstractMessageListenerContainer.java
#	spring-rabbit/src/main/java/org/springframework/amqp/rabbit/listener/BlockingQueueConsumer.java
#	spring-rabbit/src/main/java/org/springframework/amqp/rabbit/listener/DirectMessageListenerContainer.java
#	spring-rabbit/src/main/java/org/springframework/amqp/rabbit/listener/SimpleMessageListenerContainer.java
#	spring-rabbit/src/test/java/org/springframework/amqp/rabbit/listener/DirectMessageListenerContainerIntegrationTests.java
garyrussell added a commit that referenced this issue Jan 13, 2021
garyrussell added a commit that referenced this issue Jan 13, 2021
garyrussell added a commit that referenced this issue Jan 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants