Remove noisy printf in NextReader() and beginMessage()

[bcreane]

What type of PR is this? (check all applicable)

• Refactor
• Feature
• Bug Fix
• Optimization
• Documentation Update
• Go Version Update
• Dependency Update

Description

A recent check in causes NextReader() and beginMessage() to emit noisy, non-actionable messages that fill up application logs. This PR restores the previous approach of ignoring reader/writer close errors. This should address #852.

Related Tickets & Documents

• Related Issue #
• Closes [BUG] Debug messages are printed to log #852

Added/updated tests?

• Yes
• No, and this is why: no tests exist to track spurious log entries, so eliding those entries doesn't require a test update
• I need help with writing tests

Run verifications and test

• make verify is passing
• make test is passing

[bcreane]

I realize that ignoring these errors may not be ideal, however there's a note from an earlier contributor that points out that returning these errors is not feasible w/o breaking the current API contract:

 // Close previous writer if not already closed by the application. It's
 // probably better to return an error in this situation, but we cannot
 // change this without breaking existing applications.

[ghost]

Regarding c.reader.Close()

The call invokes messageReader.Close or flateReadWrapper.Close.

messageReader.Close always returns nil.

flatReadWrapper.Close returns io.ErrClosedPipe when closed previously. This error is safe to ignore. If the read wrapper is not closed, then flatReadWrapper.Close calls the flat reader Close method. The error returned from the flat Reader Close method is safe to ignore because that error would have been handled in a previous call to Read.

Summary: it's safe to ignore the error returned from c.reader.Close().

There is another call to flatReadWrapper.Close here. That code should also ignore the returned error.

Regarding c.writer.Close()

The comment says that returning the error will break existing applications. Prior to the addition of that comment, the package returned the error from cleaning up the previous message.

I believe the problem is that flatWriteWrapper.Close returns an error when called more than once. That error should be ignored, but now all errors are ignored. Perhaps that specific error should be ignored and all other errors should be returned.

Summary:

• Ignore error retuned from r.Close() in flatReadWrapper.Read.

• Consider returning all errors from the call to c.writer.Close() except errWriteClosed.

Edit: The c.writer.Close error was ignored for years. There's no indication in the issues that this caused any problems. The safe course of action is to remove the the calls to log.Printf (the two calls this PR and the call in flatReadWrapper.Read).

[bcreane]

@pennystevens thank you for the thoughtful comments - I learned a bit more about the internals as a result of you leading me through the various interfaces and their behaviors. I elected to go with the "safe" approach which is to continue to ignore the errors returned via c.writer.Close().

[ghost]

More on c.writer.Close():

The first write error on the connection is saved in Conn.writeErr.

If c.writer.Close() returned an error because write to the connection failed, then the error is saved in Conn.writerErr.

Conn.beginMessage returns that saved error.

It is therefore safe to ignore the error returned from c.writer.Close().

[DavidJohnSmith]

I also find this problem,thanks for your commit

[codecov]

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (629990d) 69.99% compared to head (bcd4e9e) 70.52%.
Report is 3 commits behind head on main.

Files	Patch %	Lines
conn.go	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #878      +/-   ##
==========================================
+ Coverage   69.99%   70.52%   +0.52%     
==========================================
  Files          11       11              
  Lines        1593     1591       -2     
==========================================
+ Hits         1115     1122       +7     
+ Misses        364      358       -6     
+ Partials      114      111       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.