Skip to content

Conversation

@cshannon
Copy link
Contributor

@cshannon cshannon commented Nov 5, 2025

This commit updates the previous fix for preventing a read check Timer
leak by the inactivity monitor when TCP connections fail
This is a follow on to #1119

This updated version has the following improvements:

  1. The logic to cancel timers and shutdown the executor has been
    consolidated into one location and only done during stopping of the
    transport. Previously, the update could stop the task/executor during
    normal broker operation (see next point)
  2. stopConnectCheckTask() is called under normal circumstances during
    initialization of a connection and the previous change caused the read
    task to be cancelled and the executor to be stopped, only to be
    immediately restarted again during the first connection creation.
  3. Null checks were added to guarantee no null pointer issues when
    referencing tasks/timers during stop
  4. Helper methods and some comments were added to simplify the code
    and make it easier to understand what is going on.

…onnection fails (apache#1119)

* Adding test.

* Naive fix.

* Fixing OpenWireConnectionTimeoutTest.
- It seems checking configuredOk can't be done that early.
- Falling back to let the Timer be created and make sure it is disposed of.
@cshannon cshannon requested a review from jbonofre November 5, 2025 13:40
@cshannon cshannon self-assigned this Nov 5, 2025
@cshannon cshannon requested a review from mattrpav November 5, 2025 13:41
This commit updates the previous fix for preventing a read check Timer
leak by the inactivity monitor when TCP connections fail
This is a follow on to apache#1119

This updated version has the following improvements:

  1) The logic to cancel timers and shutdown the executor has been
consolidated into one location and only done during stopping of the
transport. Previously, the update could stop the task/executor during
normal broker operation (see next point)
  2) stopConnectCheckTask() is called under normal circumstances during
initialization of a connection and the previous change caused the read
task to be cancelled and the executor to be stopped, only to be
immediately restarted again during the first connection creation.
  3) Null checks were added to guarantee no null pointer issues when
referencing tasks/timers during stop
  4) Helper methods and some comments were added to simplify the code
and make it easier to understand what is going on.
Copy link
Member

@jbonofre jbonofre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much cleaner approach. Thanks !

@jbonofre
Copy link
Member

jbonofre commented Nov 5, 2025

I'm just waiting Jenkins before merging (it worked locally on my machine).

This test randomly creates 250 different types of connections to verify
the read/write tasks are cancelled after all the connections have been
cleaned up
@cshannon cshannon merged commit 9dbdf61 into apache:main Nov 5, 2025
Copy link
Contributor

@mattrpav mattrpav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

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.

4 participants