syncapi: don't return early for no-op incremental syncs (#2473)

* syncapi: don't return early for no-op incremental syncs

Comments explain why, but basically it's an inefficient use
of bandwidth and some sytests rely on /sync to block.

* Honour timeouts

* Actually return a response with timeout=0

syncapi/sync/requestpool.go

@@ -251,8 +251,12 @@ func (rp *RequestPool) OnIncomingSyncRequest(req *http.Request, device *userapi.
 	waitingSyncRequests.Inc()
 	defer waitingSyncRequests.Dec()
 
+	// loop until we get some data
+	for {
+		startTime := time.Now()
 		currentPos := rp.Notifier.CurrentPosition()
 
+		// if the since token matches the current positions, wait via the notifier
 		if !rp.shouldReturnImmediately(syncReq, currentPos) {
 			timer := time.NewTimer(syncReq.Timeout) // case of timeout=0 is handled above
 			defer timer.Stop()
@@ -365,6 +369,27 @@ func (rp *RequestPool) OnIncomingSyncRequest(req *http.Request, device *userapi.
 					syncReq.Since.PresencePosition, currentPos.PresencePosition,
 				),
 			}
+			// it's possible for there to be no updates for this user even though since < current pos,
+			// e.g busy servers with a quiet user. In this scenario, we don't want to return a no-op
+			// response immediately, so let's try this again but pretend they bumped their since token.
+			// If the incremental sync was processed very quickly then we expect the next loop to block
+			// with a notifier, but if things are slow it's entirely possible that currentPos is no
+			// longer the current position so we will hit this code path again. We need to do this and
+			// not return a no-op response because:
+			// - It's an inefficient use of bandwidth.
+			// - Some sytests which test 'waking up' sync rely on some sync requests to block, which
+			//   they weren't always doing, resulting in flakey tests.
+			if !syncReq.Response.HasUpdates() {
+				syncReq.Since = currentPos
+				// do not loop again if the ?timeout= is 0 as that means "return immediately"
+				if syncReq.Timeout > 0 {
+					syncReq.Timeout = syncReq.Timeout - time.Since(startTime)
+					if syncReq.Timeout < 0 {
+						syncReq.Timeout = 0
+					}
+					continue
+				}
+			}
 		}
 
 		return util.JSONResponse{
@@ -372,6 +397,7 @@ func (rp *RequestPool) OnIncomingSyncRequest(req *http.Request, device *userapi.
 			JSON: syncReq.Response,
 		}
 	}
+}
 
 func (rp *RequestPool) OnIncomingKeyChangeRequest(req *http.Request, device *userapi.Device) util.JSONResponse {
 	from := req.URL.Query().Get("from")

syncapi/types/types.go

@@ -350,6 +350,19 @@ type Response struct {
 	DeviceListsOTKCount map[string]int `json:"device_one_time_keys_count,omitempty"`
 }
 
+func (r *Response) HasUpdates() bool {
+	// purposefully exclude DeviceListsOTKCount as we always include them
+	return (len(r.AccountData.Events) > 0 ||
+		len(r.Presence.Events) > 0 ||
+		len(r.Rooms.Invite) > 0 ||
+		len(r.Rooms.Join) > 0 ||
+		len(r.Rooms.Leave) > 0 ||
+		len(r.Rooms.Peek) > 0 ||
+		len(r.ToDevice.Events) > 0 ||
+		len(r.DeviceLists.Changed) > 0 ||
+		len(r.DeviceLists.Left) > 0)
+}
+
 // NewResponse creates an empty response with initialised maps.
 func NewResponse() *Response {
 	res := Response{}