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

feat(plc4j) Better handling of timeouts in plc4j (#821). #822

Merged
merged 1 commit into from Jun 3, 2023

Conversation

splatch
Copy link
Contributor

@splatch splatch commented Feb 23, 2023

The tricky part I was not able to solve yet is how to wire custom timeout manager into the driver. The configuration instances we have are not well defined and passing them around is not clear to me.

@spnettec
Copy link

spnettec commented Feb 26, 2023

The tricky part I was not able to solve yet is how to wire custom timeout manager into the driver. The configuration instances we have are not well defined and passing them around is not clear to me.

I think we can config in the driver

    @ConfigurationParameter("timeout-request")
    @IntDefaultValue(4000)
    protected int timeoutRequest;

    @Override
    public TimeoutManager getTimeoutManager() {
        return new TimeoutManager(){

            @Override
            public CompletionCallback register(TimedOperation operation) {
                // return null;
                // here do something
            }

            @Override
            public void stop() {
                 // here do something
            }
        };
    }

And send request like this

transaction.submit(() -> context.sendRequest(tpktPacket)
            .onTimeout(new TransactionErrorCallback<>(future, transaction))
            .onError(new TransactionErrorCallback<>(future, transaction))
            .expectResponse(TPKTPacket.class, Duration.ofMillis(configuration.getTimeoutRequest()))
            .check(p -> p.getPayload() instanceof COTPPacketData)
            .unwrap(p -> ((COTPPacketData) p.getPayload()))
            .unwrap(COTPPacket::getPayload)
            .check(p -> p.getTpduReference() == tpduId)

} else if (evt instanceof CloseConnectionEvent) {
this.protocolBase.close(new DefaultConversationContext<>(ctx, authentication, passive));
this.protocolBase.close(new DefaultConversationContext<>(this::registerHandler, ctx, authentication, passive));

Choose a reason for hiding this comment

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

Here we should stop TimeManager

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, however I am not entirely sure of netty vs plc4x lifecycle. Namely can we reestablish connection once we reached that point? At high level plc4x drivers support connect call, we would need to assure that it always configure fresh pipeline and uses new timeout manager forcing netty to fire bootstrap procedure again.

Choose a reason for hiding this comment

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

Fresh pipeline finnaly always invoke PlcConnectionManager.getConnection it will create fesh pipeline. If you don't stop the TimeManager it will caouse memory leak

Copy link
Contributor Author

@splatch splatch left a comment

Choose a reason for hiding this comment

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

Thanks for initial review, were you able to test this PR against your environment?

@spnettec
Copy link

Thanks for initial review, were you able to test this PR against your environment?

What's you changed?

@splatch splatch requested a review from spnettec June 3, 2023 21:27
Signed-off-by: Łukasz Dywicki <luke@code-house.org>
@splatch splatch merged commit efbb79c into develop Jun 3, 2023
17 checks passed
@splatch splatch deleted the issues/821 branch June 3, 2023 22:15
@splatch
Copy link
Contributor Author

splatch commented Jun 3, 2023

This PR was open for quite long time. I'm going to address @spnettec review comments in separate change set.

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

2 participants