Skip to content

add changes to release resources when the channel becomes inactive #2241

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

add changes to release resources when the channel becomes inactive #2241

wants to merge 2 commits into from

Conversation

frankpuppa
Copy link

While I was looking into this issue I found out that there are two leaks. The first is related to the RequestTransactionManager and the other is from the NettyHashTimerTimeoutManager. As I was mentioning in the issue I opened last week, this leaks happens when the PLC goes offline and back online and the client tries to reconnecting to it in a cycle.

I was only able to test this on the modbus simulator which seems to work ok with the changes I made on this branch.
Please provide me a feedback or any consideration. I am attaching also a small video which shows that the resources are freed as expected.

Issue link

Screencast.2025-09-03.09.58.05.mp4

@sruehl sruehl requested a review from chrisdutz September 3, 2025 09:23
@chrisdutz
Copy link
Contributor

Hi Frank,
do I understand things correctly? The main difference is that you're overriding channelInactive in Plc4xNettyWrapper and forwarding that to the individual implementations?
Definitely looks sensible.

Chris

@chrisdutz
Copy link
Contributor

Could you please increase the log level for your additional log output to something "debug" or "trace"? "info" makes the drivers quite noisy.

chrisdutz
chrisdutz requested changes Sep 3, 2025
View reviewed changes
plc4j/spi/src/main/java/org/apache/plc4x/java/spi/Plc4xNettyWrapper.java Outdated

@Override
public void channelInactive(ChannelHandlerContext ctx) throws Exception {
logger.info("channelInactive.. context: {}", ctx.name());
Copy link
Contributor

Choose a reason for hiding this comment

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

Please set this to "debug" or "trace"

plc4j/spi/src/test/java/org/apache/plc4x/java/spi/connection/DefaultNettyPlcConnectionTest.java

@Override
public void channelInactive(ConversationContext<Message> context) {
logger.info("On ChannelInactive");
Copy link
Contributor

Choose a reason for hiding this comment

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

Please set this to "debug" or "trace"

@frankpuppa
Copy link
Author

Hi Frank, do I understand things correctly? The main difference is that you're overriding channelInactive in Plc4xNettyWrapper and forwarding that to the individual implementations? Definitely looks sensible.

Chris

Hello Chris,
Yes exactly, basically when the plc goes offline a channelInactive event is propagated. Within the nettywrapper I basically propagate the event in the implementations which can eventually shutdown the RequestTransactionManager and release the resources. I was able to test the functionality only for the ModbusTcpProtocol. I will fix the verbosity asap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrisdutz chrisdutz chrisdutz requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@frankpuppa @chrisdutz