Skip to content

Conversation

@peterjhill
Copy link
Contributor

fix(netconf): harden getRpcReply against hangs and add robust tests

  • Reimplemented getRpcReply() to:
    • Use raw byte reads instead of Reader to avoid blocking on UTF-8 boundaries
    • Handle remote EOF cleanly with explicit NetconfException
    • Detect SSH channel closure and abort gracefully
    • Enforce commandTimeout via monotonic deadline checks
    • Add small sleep to avoid tight spin when no data is available

  • Updated tests:
    • Added getRpcReplyReturnsBodyUpToPrompt to verify normal path
    • Added getRpcReplyThrowsWhenEofBeforePrompt to verify EOF handling
    • Added getRpcReplyTimesOutOnStall to verify timeout behavior
    • Adjusted tests to include proper handshake before exercising getRpcReply
    • Allowed EOF vs timeout differences in assertions for deterministic but flexible checks

These changes prevent getRpcReply from hanging indefinitely when the remote
device closes or stalls, and expand coverage of failure and timeout cases.

for (;;) {
// First, consume any bytes that are already buffered in the stream
int avail = 0;
try { avail = in.available(); } catch (IOException ioe) { throw ioe; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Catching and re-throwing the same exception has no purpose; why do it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

totally fair! removing try/catch

int avail = 0;
try { avail = in.available(); } catch (IOException ioe) { throw ioe; }

if (avail > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm "if there is nothing to read, sleep for a bit and try again". Did you see my MR at #79 which uses a timeout on a future, so will work without any enforced delays?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did look at MR #79. A Future.get(timeout) looks nicer on paper, but for JSch it doesn’t remove enforced delays unless we’re okay with tearing down the channel:
• Timeout on a Future doesn’t unblock InputStream.read()
With JSch, the reader thread typically blocks in InputStream.read(). Timing out the caller via Future.get(timeout) just throws on the waiting thread, the reader thread is still blocked. The only reliable way to wake it is to close the channel, which is destructive for a long-lived session.

•	“No enforced delay” is only true if the underlying read is interruptible

Since JSch’s read isn’t, a Future adds orchestration, but not lower latency, unless we also add logic to close/replace the channel on each timeout.

•	Current loop drains immediately, only pauses when there’s truly nothing to read

We use in.available() to drain whatever JSch has already buffered without blocking and we break as soon as we hit the device prompt. The sleep(10ms) happens only when the buffer is empty; it avoids a tight spin and doesn’t gate reads when data is flowing.

•	Why not a blocking read with a socket/channel timeout?

JSch doesn’t give us a clean per-read timeout that’s guaranteed to interrupt the blocking read without closing the channel, which is why we avoid it here.

That said, I’m happy to make this even more responsive:
• Reduce the pause (e.g., LockSupport.parkNanos(1_000_000) for ~1ms) or adaptive backoff (1–10ms) to lower worst-case wait when the device is trickling bytes.
• Keep the “drain-immediately when available()>0” behavior so we don’t add latency when data is arriving.

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 you missed something in my PR; it ensures that the reading thread is stopped by interrupting it.

I've updated the PR to make this a bit more explicit;

  • The thread now logs if there is an error reading from the input stream
  • There's now an explicit test that demonstrates that the input stream is still valid

The new test, ifTheDeviceDoesNotRespondTheSessionCanStillBeUsed, will;

  • Create a new session
  • Send a command, and timeout expecting the reply
  • Send another command, and get the expected reply - demonstrating that the session is still good to use.

Copy link
Contributor

Choose a reason for hiding this comment

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

My main concern with this PR is that on an MX104, for example, we're seeing a commit time of ~90 seconds. I'd rather not spend those 90 seconds in a busy loop waiting for that response. And if you've got two concurrent commits on different sessions, they are queued sequentially by the device so the second commit can take ~180 seconds to respond.

Yes, it's possible to ameliorate this to an extent with a clever back-off algorithm but fundamentally it's still a busy-loop which is unnecessary.

- Reimplemented getRpcReply() to:
  • Use raw byte reads instead of Reader to avoid blocking on UTF-8 boundaries
  • Handle remote EOF cleanly with explicit NetconfException
  • Detect SSH channel closure and abort gracefully
  • Enforce commandTimeout via monotonic deadline checks
  • Add small sleep to avoid tight spin when no data is available

- Updated tests:
  • Added getRpcReplyReturnsBodyUpToPrompt to verify normal path
  • Added getRpcReplyThrowsWhenEofBeforePrompt to verify EOF handling
  • Added getRpcReplyTimesOutOnStall to verify timeout behavior
  • Adjusted tests to include proper <hello> handshake before exercising getRpcReply

squash me
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