Code review comment for lp:~pedronis/go-dbus/atomic-write-per-conn

Thank you.

Review: approve
On 19 Nov 2014 19:15, "Samuele Pedroni" <email address hidden>
wrote:

> Samuele Pedroni has proposed merging
> lp:~pedronis/go-dbus/atomic-write-per-conn into lp:go-dbus/v1.
>
> Commit message:
> make message writes on a connection atomic (not globally)
>
> Requested reviews:
> Go D-Bus Hackers (go-dbus)
>
> For more details, see:
>
> https://code.launchpad.net/~pedronis/go-dbus/atomic-write-per-conn/+merge/242257
>
> make message writes on a connection atomic (not globally)
> --
> Your team Go D-Bus Hackers is requested to review the proposed merge of
> lp:~pedronis/go-dbus/atomic-write-per-conn into lp:go-dbus/v1.
>
> === modified file 'auth.go'
> --- auth.go 2014-10-22 21:48:27 +0000
> +++ auth.go 2014-11-19 19:15:16 +0000
> @@ -124,7 +124,7 @@
> inStream := bufio.NewReader(conn)
> send := func(command ...[]byte) ([][]byte, error) {
> msg := bytes.Join(command, []byte(" "))
> - // writing at this point doesn't not need to be synced as
> the connection
> + // writing at this point does not need to be synced as the
> connection
> // is not shared at this point.
> _, err := conn.Write(append(msg, []byte("\r\n")...))
> if err != nil {
> @@ -182,7 +182,7 @@
> return errors.New("Could not authenticate with any
> mechanism")
> }
> // XXX: UNIX FD negotiation would go here.
> - // writing at this point doesn't not need to be synced as the
> connection
> + // writing at this point does not need to be synced as the
> connection
> // is not shared at this point.
> if _, err := conn.Write([]byte("BEGIN\r\n")); err != nil {
> return err
>
> === modified file 'dbus.go'
> --- dbus.go 2014-11-13 10:14:09 +0000
> +++ dbus.go 2014-11-19 19:15:16 +0000
> @@ -37,6 +37,7 @@
> // The unique name of this connection on the message bus.
> UniqueName string
> conn net.Conn
> + writeLock sync.Mutex
> busProxy BusDaemon
> lastSerial uint32
>
> @@ -251,12 +252,16 @@
> return atomic.AddUint32(&p.lastSerial, 1)
> }
>
> +func (p *Connection) atomicWriteMessage(msg *Message) error {
> + p.writeLock.Lock()
> + defer p.writeLock.Unlock()
> + _, err := msg.WriteTo(p.conn)
> + return err
> +}
> +
> func (p *Connection) Send(msg *Message) error {
> msg.setSerial(p.nextSerial())
> - if _, err := msg.WriteTo(p.conn); err != nil {
> - return err
> - }
> - return nil
> + return p.atomicWriteMessage(msg)
> }
>
> func (p *Connection) SendWithReply(msg *Message) (*Message, error) {
> @@ -272,7 +277,7 @@
> p.methodCallReplies[serial] = replyChan
> p.handlerMutex.Unlock()
>
> - if _, err := msg.WriteTo(p.conn); err != nil {
> + if err := p.atomicWriteMessage(msg); err != nil {
> p.handlerMutex.Lock()
> delete(p.methodCallReplies, serial)
> p.handlerMutex.Unlock()
>
> === modified file 'message.go'
> --- message.go 2014-10-22 16:08:20 +0000
> +++ message.go 2014-11-19 19:15:16 +0000
> @@ -4,7 +4,6 @@
> "encoding/binary"
> "errors"
> "io"
> - "sync"
> )
>
> // See the D-Bus tutorial for information about message types.
> @@ -27,8 +26,6 @@
> TypeError: "error",
> }
>
> -var writeMutex sync.Mutex
> -
> func (t MessageType) String() string { return messageTypeString[t] }
>
> type MessageFlag uint8
> @@ -320,12 +317,8 @@
> return msg, nil
> }
>
> -// WriteTo serialises the message and writes it to the given writer.
> +// WriteTo serialises the message and writes it to the given writer. Not
> atomic!
> func (p *Message) WriteTo(w io.Writer) (int64, error) {
> - // message writing needs to be sequential
> - writeMutex.Lock()
> - defer writeMutex.Unlock()
> -
> fields := make([]headerField, 0, 10)
> if p.Path != "" {
> fields = append(fields, headerField{1, Variant{p.Path}})
>
>
>