go-dbus

Merge lp:~pedronis/go-dbus/atomic-write-per-conn into lp:go-dbus/v1

  1. atomic-write-per-conn
  2. Merge into v1
Proposed by Samuele Pedroni
Status: Merged
Merged at revision: 133
Proposed branch: lp:~pedronis/go-dbus/atomic-write-per-conn
Merge into: lp:go-dbus/v1
Diff against target: 98 lines (+13/-15)
3 files modified
auth.go (+2/-2)
dbus.go (+10/-5)
message.go (+1/-8)
To merge this branch: bzr merge lp:~pedronis/go-dbus/atomic-write-per-conn
Reviewer Review Type Date Requested Status
John Lenton Approve
Review via email: mp+242257@code.launchpad.net

Commit message

make message writes on a connection atomic (not globally)

Description of the change

make message writes on a connection atomic (not globally)

To post a comment you must log in.
lp:~pedronis/go-dbus/atomic-write-per-conn updated
133. By Samuele Pedroni

fix double negations in comment

134. By Samuele Pedroni

don't lock in WriteTo itself but add doc comment

135. By Samuele Pedroni

make message writes on a connection atomic

Revision history for this message
John Lenton (chipaca) wrote :
Download full text (4.1 KiB)

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
...

Read more...

Revision history for this message
John Lenton (chipaca) wrote :
Download full text (4.3 KiB)

review approve
On 20 Nov 2014 09:01, "John Lenton" <email address hidden> wrote:

> 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.L...

Read more...

Revision history for this message
John Lenton (chipaca) wrote :

Sigh. Email interface from mobile seems iffy...

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'auth.go'
2--- auth.go 2014-10-22 21:48:27 +0000
3+++ auth.go 2014-11-19 19:15:16 +0000
4@@ -124,7 +124,7 @@
5 inStream := bufio.NewReader(conn)
6 send := func(command ...[]byte) ([][]byte, error) {
7 msg := bytes.Join(command, []byte(" "))
8- // writing at this point doesn't not need to be synced as the connection
9+ // writing at this point does not need to be synced as the connection
10 // is not shared at this point.
11 _, err := conn.Write(append(msg, []byte("\r\n")...))
12 if err != nil {
13@@ -182,7 +182,7 @@
14 return errors.New("Could not authenticate with any mechanism")
15 }
16 // XXX: UNIX FD negotiation would go here.
17- // writing at this point doesn't not need to be synced as the connection
18+ // writing at this point does not need to be synced as the connection
19 // is not shared at this point.
20 if _, err := conn.Write([]byte("BEGIN\r\n")); err != nil {
21 return err
22
23=== modified file 'dbus.go'
24--- dbus.go 2014-11-13 10:14:09 +0000
25+++ dbus.go 2014-11-19 19:15:16 +0000
26@@ -37,6 +37,7 @@
27 // The unique name of this connection on the message bus.
28 UniqueName string
29 conn net.Conn
30+ writeLock sync.Mutex
31 busProxy BusDaemon
32 lastSerial uint32
33
34@@ -251,12 +252,16 @@
35 return atomic.AddUint32(&p.lastSerial, 1)
36 }
37
38+func (p *Connection) atomicWriteMessage(msg *Message) error {
39+ p.writeLock.Lock()
40+ defer p.writeLock.Unlock()
41+ _, err := msg.WriteTo(p.conn)
42+ return err
43+}
44+
45 func (p *Connection) Send(msg *Message) error {
46 msg.setSerial(p.nextSerial())
47- if _, err := msg.WriteTo(p.conn); err != nil {
48- return err
49- }
50- return nil
51+ return p.atomicWriteMessage(msg)
52 }
53
54 func (p *Connection) SendWithReply(msg *Message) (*Message, error) {
55@@ -272,7 +277,7 @@
56 p.methodCallReplies[serial] = replyChan
57 p.handlerMutex.Unlock()
58
59- if _, err := msg.WriteTo(p.conn); err != nil {
60+ if err := p.atomicWriteMessage(msg); err != nil {
61 p.handlerMutex.Lock()
62 delete(p.methodCallReplies, serial)
63 p.handlerMutex.Unlock()
64
65=== modified file 'message.go'
66--- message.go 2014-10-22 16:08:20 +0000
67+++ message.go 2014-11-19 19:15:16 +0000
68@@ -4,7 +4,6 @@
69 "encoding/binary"
70 "errors"
71 "io"
72- "sync"
73 )
74
75 // See the D-Bus tutorial for information about message types.
76@@ -27,8 +26,6 @@
77 TypeError: "error",
78 }
79
80-var writeMutex sync.Mutex
81-
82 func (t MessageType) String() string { return messageTypeString[t] }
83
84 type MessageFlag uint8
85@@ -320,12 +317,8 @@
86 return msg, nil
87 }
88
89-// WriteTo serialises the message and writes it to the given writer.
90+// WriteTo serialises the message and writes it to the given writer. Not atomic!
91 func (p *Message) WriteTo(w io.Writer) (int64, error) {
92- // message writing needs to be sequential
93- writeMutex.Lock()
94- defer writeMutex.Unlock()
95-
96 fields := make([]headerField, 0, 10)
97 if p.Path != "" {
98 fields = append(fields, headerField{1, Variant{p.Path}})

Subscribers

People subscribed via source and target branches