Errors are not saved on close-session error #141

New issue

Open

opened 2026-03-02 14:11:46 +00:00 by robege-eri · 4 comments

robege-eri commented

2026-03-02 14:11:46 +00:00

(Migrated from github.com)

When the session is closed, the "close-session" netconf message is sent.
If any netconf errors occurs during the close, they are not propagated to the user.

In session.go (Close) we have:

// This may fail so save the error but still close the underlying transport.
req := NewRPC(&closeSession{})
resp, _ := s.Do(ctx, req)
if resp != nil {
    _ = resp.Close()
}

The comment indicates that errors from this operation are saved, but there are no traces of this in the code.
The resp variable is not used anywhere else in the code.

If this code is replaced by;

// This may fail so save the error but still close the underlying transport.
closeErr = s.Exec(ctx, &closeSession{}, nil)

I suppose by doing this, the errors should be extracted from the response and saved in closeErr ?

When the session is closed, the "close-session" netconf message is sent. If any netconf errors occurs during the close, they are not propagated to the user. In session.go (Close) we have: // This may fail so save the error but still close the underlying transport. req := NewRPC(&closeSession{}) resp, _ := s.Do(ctx, req) if resp != nil { _ = resp.Close() } The comment indicates that errors from this operation are saved, but there are no traces of this in the code. The resp variable is not used anywhere else in the code. If this code is replaced by; // This may fail so save the error but still close the underlying transport. closeErr = s.Exec(ctx, &closeSession{}, nil) I suppose by doing this, the errors should be extracted from the response and saved in closeErr ?

nemith commented

2026-03-02 16:45:19 +00:00

(Migrated from github.com)

I remember this being somewhat intentional in an effort to simplify the close operation (but i forgot to remove the comment).

Is this something found during code review or some functionality you need? Do you know what functionality you would desire here?

I remember this being somewhat intentional in an effort to simplify the close operation (but i forgot to remove the comment). Is this something found during code review or some functionality you need? Do you know what functionality you would desire here?

robege-eri commented

2026-03-02 19:36:04 +00:00

(Migrated from github.com)

Hello Brandon, and thanks for the swift reply!

Yes, we have in some cases seen that when doing a EditConfig operation towards a running configuration, that operation is successful but errors are reported on a subsequential Close of the session. These errors are now lost, since the body of the close-session response is completely ignored. With the change above, those errors would be propagated to the user
doing the close (after closing the underlying sockets). Would this break anything else?

Hello Brandon, and thanks for the swift reply! Yes, we have in some cases seen that when doing a EditConfig operation towards a running configuration, that operation is successful but errors are reported on a subsequential Close of the session. These errors are now lost, since the body of the close-session response is completely ignored. With the change above, those errors would be propagated to the user doing the close (after closing the underlying sockets). Would this break anything else?

nemith commented

2026-03-03 02:28:55 +00:00

(Migrated from github.com)

Shouldn't break anything. It will break some unit tests but that is a problem with the unit tests and not the implementation.

robege-eri commented

2026-03-03 07:54:08 +00:00

(Migrated from github.com)

I ran the unit tests with this change, and they still passed from what I could see.