Skip to content

#62 getRpcReply hang issue is fixed. #63

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

beserb
Copy link

@beserb beserb commented Jul 15, 2022

No description provided.

int charsRead = in.read(buffer, 0, buffer.length);
if (charsRead < 0) throw new NetconfException("Input Stream has been closed during reading.");
rpcReply.append(buffer, 0, charsRead);
if (in.ready()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a bad idea. It looks to me it will end up in a tight loop burning CPU until timeout or the device sends some data.

Copy link
Author

Choose a reason for hiding this comment

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

You are right. I have investigated a bit further and it seems like having a timeout control using another thread is a valid solution. I have implemented it, now all tests pass and hang issue seems resolved. However, as I don't have much experience with threads I will investigate this solution a bit more and commit later on.

Do you have any other idea or any objection?

Copy link
Contributor

Choose a reason for hiding this comment

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

Without thinking about it too much, a CompletableFuture is probably what you want to be looking at, but there are no doubt other options.

Copy link
Contributor

Choose a reason for hiding this comment

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

(Though there may well be a better way using NIO, but can’t remember the full context of this code off the top of my head , so unsure if it could easily be applied in this case)

@beserb beserb marked this pull request as draft July 21, 2022 19:48
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.

2 participants