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.

Sign up for GitHub

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

Fix window update, reduce memory utilization and improve performance #50

Closed
wants to merge 7 commits into from
Closed

Fix window update, reduce memory utilization and improve performance #50

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
aa942b4
window update unit test for partial read; benchmark large buffer
stuartcarnie Sep 12, 2017
6d85d98
improve memory utilization in receive buffer, fix flow control
stuartcarnie Sep 12, 2017
e7f9152
Add typeMax constant to define range.
stuartcarnie Sep 13, 2017
49b0a1e
Fix race condition when SYN is successfully sent
stuartcarnie Nov 3, 2017
a40d23b
Merge pull request #1 from influxdata/sgc-SYN-race
stuartcarnie Nov 3, 2017
63c6e7e
not yamux consistency issues, log as WARN
stuartcarnie Nov 7, 2017
1f58ded
Merge pull request #2 from influxdata/sgc-warn
stuartcarnie Nov 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions session.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ func (s *Session) send() {
for sent < len(ready.Hdr) {
n, err := s.conn.Write(ready.Hdr[sent:])
if err != nil {
s.logger.Printf("[ERR] yamux: Failed to write header: %v", err)
s.logger.Printf("[WARN] yamux: Failed to write header: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why exactly should it be a warning and not an error? If I understand correctly this will completely stop the yamux connection. I would think with just a warning things would be able to recover and continue to work, not stop completely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@erikdubbelboer none of those additional commits should have appeared in this PR. We are maintaining our own fork.

An error is still returned to the client, this only changes a log message. I consider yamux consistency issues bugs and should be logged as errors. An example of a consistency issue would yamux: SYN tracking out of sync and highlights a bug in yamux. The network connection being closed, causing a write to fail is not a yamux bug and is still reported as an error to the client code, however it generates unnecessary log noise.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case should it log anything at all? Maybe it should just return the error and let the application log something if it wants. I always thing libraries logging things if only for when it can't tell its user that something happened (by returning an error for example).

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 definitely agree. The logging within the yamux package should be restricted to yamux internal consistency issues. Logging a warning because the network connection has been closed is redundant, since the error will be passed to the caller

asyncSendErr(ready.Err, err)
s.exitErr(err)
return
Expand All @@ -387,7 +387,7 @@ func (s *Session) send() {
if ready.Body != nil {
_, err := io.Copy(s.conn, ready.Body)
if err != nil {
s.logger.Printf("[ERR] yamux: Failed to write body: %v", err)
s.logger.Printf("[WARN] yamux: Failed to write body: %v", err)
asyncSendErr(ready.Err, err)
s.exitErr(err)
return
Expand Down