Repository: godbus/dbus
Pull request #214 (2020-10-13 05:54:07)

conn: centralise (most) serial handling to sendMessageAndIfClosed
Previously all code creating messages also had to assign serial numbers to those messages.  With this change, `Conn.sendMessageAndIfClosed()` will assign a serial number to any message whose serial number is 0 (which is
invalid as per the spec).

The `Conn.send()` method (used to send method calls) still manually sets the serial number so it can set up the Call struct to receive the response.

`Object.createCall()` has also been simplified to make use of `Conn.SendWithContext` rather than repeating its logic.

jhenstridge: Looks like Travis isn't posting check suite results for some reason, but here's the run:

https://travis-ci.org/github/godbus/dbus/builds/730079181

I don't think the Go 1.13 failure is due to my changes.  I have seen the following failure once though (not easily reproducible):

```
...
--- PASS: TestSequentialHandler_TerminateTerminates (0.10s)
=== CONT  TestSequentialHandler_RemoveAfterTerminate
--- PASS: TestSequentialHandler_RemoveAfterTerminate (0.10s)
=== CONT  TestSequentialHandlerSequential
race: limit on 8128 simultaneously alive goroutines is exceeded, dying
FAIL	github.com/godbus/dbus/v5	6.471s
```

From the look of it, a number of these tests will spawn (at least) 1000 goroutines, and are being run in parallel.  So it's not surprising it could sometimes hit that limit.
jsouthworth: > Looks like Travis isn't posting check suite results for some reason, but here's the run:
> 
> https://travis-ci.org/github/godbus/dbus/builds/730079181
> 
> I don't think the Go 1.13 failure is due to my changes. I have seen the following failure once though (not easily reproducible):
> 
> ```
> ...
> --- PASS: TestSequentialHandler_TerminateTerminates (0.10s)
> === CONT  TestSequentialHandler_RemoveAfterTerminate
> --- PASS: TestSequentialHandler_RemoveAfterTerminate (0.10s)
> === CONT  TestSequentialHandlerSequential
> race: limit on 8128 simultaneously alive goroutines is exceeded, dying
> FAIL	github.com/godbus/dbus/v5	6.471s
> ```
> 
> From the look of it, a number of these tests will spawn (at least) 1000 goroutines, and are being run in parallel. So it's not surprising it could sometimes hit that limit.

OK, I'll take a look at this problem.

Patch:
diff --git a/conn.go b/conn.go
index f959d21..664bc62 100644
--- a/conn.go
+++ b/conn.go
@@ -464,6 +464,9 @@ func (conn *Conn) Object(dest string, path ObjectPath) BusObject {
 }
 
 func (conn *Conn) sendMessageAndIfClosed(msg *Message, ifClosed func()) {
+	if msg.serial == 0 {
+		msg.serial = conn.getSerial()
+	}
 	if conn.outInt != nil {
 		conn.outInt(msg)
 	}
@@ -495,16 +498,16 @@ func (conn *Conn) send(ctx context.Context, msg *Message, ch chan *Call) *Call {
 	if ctx == nil {
 		panic("nil context")
 	}
+	if ch == nil {
+		ch = make(chan *Call, 1)
+	} else if cap(ch) == 0 {
+		panic("dbus: unbuffered channel passed to (*Conn).Send")
+	}
 
 	var call *Call
 	ctx, canceler := context.WithCancel(ctx)
 	msg.serial = conn.getSerial()
 	if msg.Type == TypeMethodCall && msg.Flags&FlagNoReplyExpected == 0 {
-		if ch == nil {
-			ch = make(chan *Call, 5)
-		} else if cap(ch) == 0 {
-			panic("dbus: unbuffered channel passed to (*Conn).Send")
-		}
 		call = new(Call)
 		call.Destination, _ = msg.Headers[FieldDestination].value.(string)
 		call.Path, _ = msg.Headers[FieldPath].value.(ObjectPath)
@@ -526,7 +529,8 @@ func (conn *Conn) send(ctx context.Context, msg *Message, ch chan *Call) *Call {
 		})
 	} else {
 		canceler()
-		call = &Call{Err: nil}
+		call = &Call{Err: nil, Done: ch}
+		ch <- call
 		conn.sendMessageAndIfClosed(msg, func() {
 			call = &Call{Err: ErrClosed}
 		})
@@ -551,7 +555,6 @@ func (conn *Conn) sendError(err error, dest string, serial uint32) {
 	}
 	msg := new(Message)
 	msg.Type = TypeError
-	msg.serial = conn.getSerial()
 	msg.Headers = make(map[HeaderField]Variant)
 	if dest != "" {
 		msg.Headers[FieldDestination] = MakeVariant(dest)
@@ -570,7 +573,6 @@ func (conn *Conn) sendError(err error, dest string, serial uint32) {
 func (conn *Conn) sendReply(dest string, serial uint32, values ...interface{}) {
 	msg := new(Message)
 	msg.Type = TypeMethodReply
-	msg.serial = conn.getSerial()
 	msg.Headers = make(map[HeaderField]Variant)
 	if dest != "" {
 		msg.Headers[FieldDestination] = MakeVariant(dest)
diff --git a/export.go b/export.go
index 1093b01..bfc5db6 100644
--- a/export.go
+++ b/export.go
@@ -175,7 +175,6 @@ func (conn *Conn) handleCall(msg *Message) {
 	if msg.Flags&FlagNoReplyExpected == 0 {
 		reply := new(Message)
 		reply.Type = TypeMethodReply
-		reply.serial = conn.getSerial()
 		reply.Headers = make(map[HeaderField]Variant)
 		if hasSender {
 			reply.Headers[FieldDestination] = msg.Headers[FieldSender]
@@ -211,7 +210,6 @@ func (conn *Conn) Emit(path ObjectPath, name string, values ...interface{}) erro
 	}
 	msg := new(Message)
 	msg.Type = TypeSignal
-	msg.serial = conn.getSerial()
 	msg.Headers = make(map[HeaderField]Variant)
 	msg.Headers[FieldInterface] = MakeVariant(iface)
 	msg.Headers[FieldMember] = MakeVariant(member)
diff --git a/object.go b/object.go
index 6dc10dc..664abb7 100644
--- a/object.go
+++ b/object.go
@@ -110,7 +110,6 @@ func (o *Object) createCall(ctx context.Context, method string, flags Flags, ch
 	method = method[i+1:]
 	msg := new(Message)
 	msg.Type = TypeMethodCall
-	msg.serial = o.conn.getSerial()
 	msg.Flags = flags & (FlagNoAutoStart | FlagNoReplyExpected)
 	msg.Headers = make(map[HeaderField]Variant)
 	msg.Headers[FieldPath] = MakeVariant(o.path)
@@ -123,47 +122,7 @@ func (o *Object) createCall(ctx context.Context, method string, flags Flags, ch
 	if len(args) > 0 {
 		msg.Headers[FieldSignature] = MakeVariant(SignatureOf(args...))
 	}
-	if msg.Flags&FlagNoReplyExpected == 0 {
-		if ch == nil {
-			ch = make(chan *Call, 1)
-		} else if cap(ch) == 0 {
-			panic("dbus: unbuffered channel passed to (*Object).Go")
-		}
-		ctx, cancel := context.WithCancel(ctx)
-		call := &Call{
-			Destination: o.dest,
-			Path:        o.path,
-			Method:      method,
-			Args:        args,
-			Done:        ch,
-			ctxCanceler: cancel,
-			ctx:         ctx,
-		}
-		o.conn.calls.track(msg.serial, call)
-		o.conn.sendMessageAndIfClosed(msg, func() {
-			o.conn.calls.handleSendError(msg, ErrClosed)
-			cancel()
-		})
-		go func() {
-			<-ctx.Done()
-			o.conn.calls.handleSendError(msg, ctx.Err())
-		}()
-
-		return call
-	}
-	done := make(chan *Call, 1)
-	call := &Call{
-		Err:  nil,
-		Done: done,
-	}
-	defer func() {
-		call.Done <- call
-		close(done)
-	}()
-	o.conn.sendMessageAndIfClosed(msg, func() {
-		call.Err = ErrClosed
-	})
-	return call
+	return o.conn.SendWithContext(ctx, msg, ch)
 }
 
 // GetProperty calls org.freedesktop.DBus.Properties.Get on the given


Test patch:
diff --git a/conn_test.go b/conn_test.go
index ff23b71..8ac32b8 100644
--- a/conn_test.go
+++ b/conn_test.go
@@ -461,7 +461,6 @@ process:
 				t.Logf("Processing reply to .State(), returning state = %v", state)
 				reply := new(Message)
 				reply.Type = TypeMethodReply
-				reply.serial = srv.getSerial()
 				reply.Headers = make(map[HeaderField]Variant)
 				reply.Headers[FieldDestination] = msg.Headers[FieldSender]
 				reply.Headers[FieldReplySerial] = MakeVariant(msg.serial)