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.

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

buffering go-msgio breaks the protocol #1304

Closed
whyrusleeping opened this issue May 29, 2015 · 4 comments
Closed

buffering go-msgio breaks the protocol #1304

whyrusleeping opened this issue May 29, 2015 · 4 comments

Comments

@whyrusleeping
Copy link
Member

I was playing around with improving performance over the network and decided to reduce the number of write syscalls we made, so i did the following patch:

diff --git a/Godeps/_workspace/src/github.com/jbenet/go-msgio/msgio.go b/Godeps/_workspace/src/github.com/jbenet/go-msgio/msgio.go
index 6547e68..09a9de0 100644
--- a/Godeps/_workspace/src/github.com/jbenet/go-msgio/msgio.go
+++ b/Godeps/_workspace/src/github.com/jbenet/go-msgio/msgio.go
@@ -1,6 +1,7 @@
 package msgio

 import (
+       "bytes"
        "encoding/binary"
        "errors"
        "io"
@@ -102,11 +103,14 @@ func (s *writer) WriteMsg(msg []byte) (err error) {
        s.lock.Lock()
        defer s.lock.Unlock()

+       buf := new(bytes.Buffer)
        length := uint32(len(msg))
-       if err := binary.Write(s.W, NBO, &length); err != nil {
+       if err := binary.Write(buf, NBO, &length); err != nil {
                return err
        }
-       _, err = s.W.Write(msg)
+       buf.Write(msg)
+
+       _, err = buf.WriteTo(s.W)
        return err
 }

It writes the data to a buffer, then dumps that buffer to the intended writer. (i know i could have used a bufio.Writer but when have i ever done things the easy way). After this patch, i cannot connect to any other node.

@whyrusleeping
Copy link
Member Author

@jbenet @cryptix @wking if anyone can point out what i'm doing wrong, that would be great.

@wking
Copy link
Contributor

wking commented May 29, 2015

On Fri, May 29, 2015 at 02:43:33PM -0700, Jeromy Johnson wrote:

  •   _, err = s.W.Write(msg)
    
  •   buf.Write(msg)
    

It's not likely to be the cause of your problem, but you're not
checking for errors here.

Other than that, this looks innocuous to me. Not sure why it's
breaking shrug

@jbenet
Copy link
Member

jbenet commented May 29, 2015

@whyrusleeping what is being output by the node? run two of them (one new, one old) and then nc | hexdump -C on both of them.

@whyrusleeping
Copy link
Member Author

This has been fixed in 0.4.0

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

No branches or pull requests

3 participants