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

New various Changes for #1 #818

Closed
wants to merge 7 commits into from

Conversation

spnettec
Copy link

fix OutOfMemoryError
add Timeout-Handling
fix connection-cache

add Timeout-HandlingVarious Changes
@chrisdutz
Copy link
Contributor

YESS! ... that's much more something we can discuss ... I'll do that as soon as I can (Currently not allowed to do Apache code-work while on the clock :-( ) ... but just wanted to say that it's greatly appreciated :-)

Copy link
Contributor

@chrisdutz chrisdutz left a comment

Choose a reason for hiding this comment

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

How do the changes from the Nett wrapper correspond to the ones in the connection-cache?

@@ -66,7 +76,7 @@ public void connect() throws PlcConnectionException {
@Override
public boolean isConnected() {
if(connection == null) {
throw new PlcRuntimeException("Error using leased connection after returning it to the cache.");
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not quite sure this is the best way to do this. Because for me there's a difference, if we're not connected or if we already gave back the handle. The way I implemented it gives the user the explicit feedback, that he's doing something bad. If we return false, he might think he just needs to re-connect.

Copy link
Author

Choose a reason for hiding this comment

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

If we lose the connection we can handle reconnection

Copy link
Author

@spnettec spnettec Feb 22, 2023

Choose a reason for hiding this comment

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

By the way. I'm working on eclipse kura. It require work uninterrupted. We must have explicit reconect signal to reconnect not a exception.

Copy link
Author

Choose a reason for hiding this comment

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

I have already explained that previous PR are the basis for subsequent PRs. Next I want to submit a PR for PL4x

Copy link
Contributor

Choose a reason for hiding this comment

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

I have encountered a scenario where the PLC connection appeared to be disconnected, but in reality it was still connected due to certain reasons. However, we kept trying to reconnect and as a result, the TCP estab value increased to tens of thousands. This caused the PLC to be disconnected and also resulted in server network issues. Therefore, I'm looking to see if there is a better solution available.

Copy link
Contributor

Choose a reason for hiding this comment

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

I still don't quite understand the problem with this ... if you returned the connection to the pool, you should not do anything with it ... that's sort of the contract on using the pool. If there's some logic calling "isConnected()" after it has been returned, that's just a flaw in the program using it. I still think we should continue to throw an exception here.

Copy link
Author

Choose a reason for hiding this comment

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

We use PLC4x to collect data for long term work. If the connection is closed abnormally, I don’t want to always use the try catch method. If we use this to detect that the connection is closed, we only need to borrow it again.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say that's a usecase particular to how you use PLC4X, however out API shouldn't be optimized towards one particular usecase. I think try-catch-blocks don't consume any noticable amount of resources. So I would like to ask to to stick to using try-catch blocks and us keeping the API coherent.

Copy link
Author

Choose a reason for hiding this comment

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

Just connect is not allowed. I think isConeccted is should reserve the functionality of the original API

Copy link
Author

Choose a reason for hiding this comment

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

I'm wondering why you want to disable this API functionality and replace it with an error

@spnettec
Copy link
Author

spnettec commented Feb 22, 2023 via email

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