From d40fbda2abd1a5fe55f1f15d418554b3052a976b Mon Sep 17 00:00:00 2001 From: Jakub Date: Mon, 13 Dec 2021 12:29:49 +0100 Subject: [PATCH] GODT-1468: Fix main windows status and add background context without retry. --- internal/frontend/qml/AccountDelegate.qml | 23 ++++- internal/frontend/qml/Bridge_test.qml | 4 +- internal/frontend/qml/ContentWrapper.qml | 9 +- .../qml/Notifications/Notifications.qml | 9 +- internal/frontend/qml/Status.qml | 99 +++---------------- internal/store/event_loop.go | 4 +- internal/store/store.go | 2 +- internal/users/user.go | 2 +- pkg/pmapi/manager.go | 13 ++- pkg/pmapi/manager_download.go | 3 +- pkg/pmapi/manager_ping.go | 31 +++++- pkg/pmapi/response.go | 14 ++- 12 files changed, 101 insertions(+), 112 deletions(-) diff --git a/internal/frontend/qml/AccountDelegate.qml b/internal/frontend/qml/AccountDelegate.qml index 24128d6b..27154d04 100644 --- a/internal/frontend/qml/AccountDelegate.qml +++ b/internal/frontend/qml/AccountDelegate.qml @@ -36,12 +36,27 @@ Item { if (root.usedFraction < .75) return root.colorScheme.signal_warning return root.colorScheme.signal_danger } - property real usedFraction: root.user && root.user.totalBytes ? Math.abs(root.user.usedBytes / root.user.totalBytes) : 0 - property string totalSpace: root.spaceWithUnits(root.user ? root.user.totalBytes : 0) - property string usedSpace: root.spaceWithUnits(root.user ? root.user.usedBytes : 0) + property real usedFraction: root.user ? reasonableFracion(root.user.usedBytes, root.user.totalBytes) : 0 + property string totalSpace: root.spaceWithUnits(root.user ? root.reasonableBytes(root.user.totalBytes) : 0) + property string usedSpace: root.spaceWithUnits(root.user ? root.reasonableBytes(root.user.usedBytes) : 0) + + function reasonableFracion(used, total){ + var usedSafe = root.reasonableBytes(used) + var totalSafe = root.reasonableBytes(total) + if (totalSafe == 0 || usedSafe == 0) return 0 + if (totalSafe <= usedSafe) return 1 + return usedSafe / totalSafe + } + + function reasonableBytes(bytes){ + var safeBytes = bytes+0 + if (safeBytes != bytes) return 0 + if (safeBytes < 0) return 0 + return Math.ceil(safeBytes) + } function spaceWithUnits(bytes){ - if (bytes*1 !== bytes ) return "0 kB" + if (bytes*1 !== bytes || bytes == 0 ) return "0 kB" var units = ['B',"kB", "MB", "GB", "TB"]; var i = parseInt(Math.floor(Math.log(bytes)/Math.log(1024))); diff --git a/internal/frontend/qml/Bridge_test.qml b/internal/frontend/qml/Bridge_test.qml index e45e5c2a..4890c540 100644 --- a/internal/frontend/qml/Bridge_test.qml +++ b/internal/frontend/qml/Bridge_test.qml @@ -242,7 +242,7 @@ Window { // add one user on start - var haveUserOnStart = false + var haveUserOnStart = true if (haveUserOnStart) { var newUserObject = root.userComponent.createObject(root) newUserObject.username = "LerooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooyJenkins@protonmail.com" @@ -808,7 +808,7 @@ Window { signal userDisconnected(string username) signal apiCertIssue() - property bool showSplashScreen: true + property bool showSplashScreen: false function login(username, password) { diff --git a/internal/frontend/qml/ContentWrapper.qml b/internal/frontend/qml/ContentWrapper.qml index bad78a49..c2775b34 100644 --- a/internal/frontend/qml/ContentWrapper.qml +++ b/internal/frontend/qml/ContentWrapper.qml @@ -20,6 +20,7 @@ import QtQuick.Layouts 1.12 import QtQuick.Controls 2.12 import Proton 4.0 +import Notifications 1.0 Item { id: root @@ -59,12 +60,16 @@ Item { spacing: 0 Status { - colorScheme: leftBar.colorScheme Layout.leftMargin: 16 Layout.topMargin: 24 Layout.bottomMargin: 17 - Layout.alignment: Qt.AlignHCenter + + colorScheme: leftBar.colorScheme + backend: root.backend + notifications: root.notifications + + notificationWhitelist: Notifications.Group.Connection | Notifications.Group.ForceUpdate } // just a placeholder diff --git a/internal/frontend/qml/Notifications/Notifications.qml b/internal/frontend/qml/Notifications/Notifications.qml index 14d05d42..0bb78db8 100644 --- a/internal/frontend/qml/Notifications/Notifications.qml +++ b/internal/frontend/qml/Notifications/Notifications.qml @@ -37,10 +37,11 @@ QtObject { signal askDeleteAccount(var user) enum Group { - Connection = 1, - Update = 2, + Connection = 1, + Update = 2, Configuration = 4, - API = 32, + ForceUpdate = 8, + API = 32, // Special group for notifications that require dialog popup instead of banner Dialogs = 64 @@ -201,7 +202,7 @@ QtObject { brief: qsTr("Bridge is outdated") icon: "./icons/ic-exclamation-circle-filled.svg" type: Notification.NotificationType.Danger - group: Notifications.Group.Update | Notifications.Group.Dialogs + group: Notifications.Group.Update | Notifications.Group.ForceUpdate | Notifications.Group.Dialogs Connections { target: root.backend diff --git a/internal/frontend/qml/Status.qml b/internal/frontend/qml/Status.qml index 488a8049..195c9ade 100644 --- a/internal/frontend/qml/Status.qml +++ b/internal/frontend/qml/Status.qml @@ -59,19 +59,19 @@ Item { label.text = topmost.brief switch (topmost.type) { - case Notification.NotificationType.Danger: + case Notification.NotificationType.Danger: image.color = root.colorScheme.signal_danger label.color = root.colorScheme.signal_danger break; - case Notification.NotificationType.Warning: + case Notification.NotificationType.Warning: image.color = root.colorScheme.signal_warning label.color = root.colorScheme.signal_warning break; - case Notification.NotificationType.Success: + case Notification.NotificationType.Success: image.color = root.colorScheme.signal_success label.color = root.colorScheme.signal_success break; - case Notification.NotificationType.Info: + case Notification.NotificationType.Info: image.color = root.colorScheme.signal_info label.color = root.colorScheme.signal_info break; @@ -85,8 +85,12 @@ Item { ColorImage { id: image - Layout.fillHeight: true + width: 16 + height: 16 + sourceSize.width: width sourceSize.height: height + source: "./icons/ic-connected.svg" + color: root.colorScheme.signal_success } Label { @@ -100,88 +104,9 @@ Item { horizontalAlignment: Text.AlignLeft verticalAlignment: Text.AlignVCenter + + text: qsTr("Connected") + color: root.colorScheme.signal_success } } - - state: "Connected" - states: [ - State { - name: "Connected" - PropertyChanges { - target: image - source: "./icons/ic-connected.svg" - color: ProtonStyle.currentStyle.signal_success - } - PropertyChanges { - target: label - text: qsTr("Connected") - color: ProtonStyle.currentStyle.signal_success - } - }, - State { - name: "No connection" - PropertyChanges { - target: image - source: "./icons/ic-no-connection.svg" - color: ProtonStyle.currentStyle.signal_danger - } - PropertyChanges { - target: label - text: qsTr("No connection") - color: ProtonStyle.currentStyle.signal_danger - } - }, - State { - name: "Outdated" - PropertyChanges { - target: image - source: "./icons/ic-exclamation-circle-filled.svg" - color: ProtonStyle.currentStyle.signal_danger - } - PropertyChanges { - target: label - text: qsTr("Bridge is outdated") - color: ProtonStyle.currentStyle.signal_danger - } - }, - State { - name: "Account changed" - PropertyChanges { - target: image - source: "./icons/ic-exclamation-circle-filled.svg" - color: ProtonStyle.currentStyle.signal_danger - } - PropertyChanges { - target: label - text: qsTr("The address list for your account has changed") - color: ProtonStyle.currentStyle.signal_danger - } - }, - State { - name: "Auto update failed" - PropertyChanges { - target: image - source: "./icons/ic-info-circle-filled.svg" - color: ProtonStyle.currentStyle.signal_info - } - PropertyChanges { - target: label - text: qsTr("Bridge couldn’t update automatically") - color: ProtonStyle.currentStyle.signal_info - } - }, - State { - name: "Update ready" - PropertyChanges { - target: image - source: "./icons/ic-info-circle-filled.svg" - color: ProtonStyle.currentStyle.signal_info - } - PropertyChanges { - target: label - text: qsTr("Bridge update is ready") - color: ProtonStyle.currentStyle.signal_info - } - } - ] } diff --git a/internal/store/event_loop.go b/internal/store/event_loop.go index 81d42889..c7d9ab8e 100644 --- a/internal/store/event_loop.go +++ b/internal/store/event_loop.go @@ -81,7 +81,7 @@ func (loop *eventLoop) client() pmapi.Client { func (loop *eventLoop) setFirstEventID() (err error) { loop.log.Info("Setting first event ID") - event, err := loop.client().GetEvent(context.Background(), "") + event, err := loop.client().GetEvent(pmapi.ContextWithoutRetry(context.Background()), "") if err != nil { loop.log.WithError(err).Error("Could not get latest event ID") return @@ -269,7 +269,7 @@ func (loop *eventLoop) processNextEvent() (more bool, err error) { // nolint[fun loop.pollCounter++ var event *pmapi.Event - if event, err = loop.client().GetEvent(context.Background(), loop.currentEventID); err != nil { + if event, err = loop.client().GetEvent(pmapi.ContextWithoutRetry(context.Background()), loop.currentEventID); err != nil { return false, errors.Wrap(err, "failed to get event") } diff --git a/internal/store/store.go b/internal/store/store.go index 6814a504..3c848289 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -312,7 +312,7 @@ func (store *Store) client() pmapi.Client { // initCounts initialises the counts for each label. It tries to use the API first to fetch the labels but if // the API is unavailable for whatever reason it tries to fetch the labels locally. func (store *Store) initCounts() (labels []*pmapi.Label, err error) { - if labels, err = store.client().ListLabels(context.Background()); err != nil { + if labels, err = store.client().ListLabels(pmapi.ContextWithoutRetry(context.Background())); err != nil { store.log.WithError(err).Warn("Could not list API labels. Trying with local labels.") if labels, err = store.getLabelsFromLocalStorage(); err != nil { store.log.WithError(err).Error("Cannot list local labels") diff --git a/internal/users/user.go b/internal/users/user.go index e6688d1e..e9fdea72 100644 --- a/internal/users/user.go +++ b/internal/users/user.go @@ -223,7 +223,7 @@ func (u *User) UpdateSpace(apiUser *pmapi.User) { // values from client.CurrentUser() if apiUser == nil { var err error - apiUser, err = u.client.GetUser(context.Background()) + apiUser, err = u.client.GetUser(pmapi.ContextWithoutRetry(context.Background())) if err != nil { u.log.WithError(err).Warning("Cannot update user space") return diff --git a/pkg/pmapi/manager.go b/pkg/pmapi/manager.go index 512b81c2..c5a76866 100644 --- a/pkg/pmapi/manager.go +++ b/pkg/pmapi/manager.go @@ -34,6 +34,9 @@ type manager struct { locker sync.Locker connectionObservers []ConnectionObserver proxyDialer *ProxyTLSDialer + + pingMutex *sync.RWMutex + isPinging bool } func New(cfg Config) Manager { @@ -42,9 +45,11 @@ func New(cfg Config) Manager { func newManager(cfg Config) *manager { m := &manager{ - cfg: cfg, - rc: resty.New().EnableTrace(), - locker: &sync.Mutex{}, + cfg: cfg, + rc: resty.New().EnableTrace(), + locker: &sync.Mutex{}, + pingMutex: &sync.RWMutex{}, + isPinging: false, } proxyDialer, transport := newProxyDialerAndTransport(cfg) @@ -75,7 +80,7 @@ func newManager(cfg Config) *manager { m.rc.SetRetryCount(30) m.rc.SetRetryMaxWaitTime(time.Minute) m.rc.SetRetryAfter(catchRetryAfter) - m.rc.AddRetryCondition(shouldRetry) + m.rc.AddRetryCondition(m.shouldRetry) return m } diff --git a/pkg/pmapi/manager_download.go b/pkg/pmapi/manager_download.go index 15f75a37..4e813e2c 100644 --- a/pkg/pmapi/manager_download.go +++ b/pkg/pmapi/manager_download.go @@ -21,6 +21,7 @@ import ( "io/ioutil" "github.com/ProtonMail/gopenpgp/v2/crypto" + "golang.org/x/net/context" ) // DownloadAndVerify downloads a file and its signature from the given locations `file` and `sig`. @@ -50,7 +51,7 @@ func (m *manager) DownloadAndVerify(kr *crypto.KeyRing, url, sig string) ([]byte } func (m *manager) fetchFile(url string) ([]byte, error) { - res, err := m.rc.R().SetDoNotParseResponse(true).Get(url) + res, err := m.r(ContextWithoutRetry(context.Background())).SetDoNotParseResponse(true).Get(url) if err != nil { return nil, err } diff --git a/pkg/pmapi/manager_ping.go b/pkg/pmapi/manager_ping.go index a47bac76..e3771109 100644 --- a/pkg/pmapi/manager_ping.go +++ b/pkg/pmapi/manager_ping.go @@ -30,20 +30,47 @@ var ( ) func (m *manager) pingUntilSuccess() { + if m.isPingOngoing() { + logrus.Debug("Ping already ongoing") + return + } + m.pingingStarted() + defer m.pingingStopped() + attempt := 0 for { - err := m.testPing(context.Background()) + ctx := ContextWithoutRetry(context.Background()) + err := m.testPing(ctx) if err == nil { return } waitTime := getRetryConnectionSleep(attempt) attempt++ - logrus.WithError(err).WithField("attempt", attempt).WithField("wait", waitTime).Debug("Connection not available") + logrus.WithError(err).WithField("attempt", attempt).WithField("wait", waitTime).Debug("Connection (still) not available") time.Sleep(waitTime) } } +func (m *manager) isPingOngoing() bool { + m.pingMutex.RLock() + defer m.pingMutex.RUnlock() + + return m.isPinging +} + +func (m *manager) pingingStarted() { + m.pingMutex.Lock() + defer m.pingMutex.Unlock() + m.isPinging = true +} + +func (m *manager) pingingStopped() { + m.pingMutex.Lock() + defer m.pingMutex.Unlock() + m.isPinging = false +} + func getRetryConnectionSleep(idx int) time.Duration { if idx >= len(retryConnectionSleeps) { idx = len(retryConnectionSleeps) - 1 diff --git a/pkg/pmapi/response.go b/pkg/pmapi/response.go index 93967f9f..612849b3 100644 --- a/pkg/pmapi/response.go +++ b/pkg/pmapi/response.go @@ -118,11 +118,21 @@ func catchRetryAfter(_ *resty.Client, res *resty.Response) (time.Duration, error return 0, nil } -func shouldRetry(res *resty.Response, err error) bool { +func (m *manager) shouldRetry(res *resty.Response, err error) bool { if isRetryDisabled(res.Request.Context()) { return false } - return isTooManyRequest(res) || isNoResponse(res, err) + if isTooManyRequest(res) { + return true + } + if isNoResponse(res, err) { + // Even if the context of request allows to retry we should check + // whether the server is reachable or not. In some cases the we can + // keep retrying but also report that connection is lost. + go m.pingUntilSuccess() + return true + } + return false } func isTooManyRequest(res *resty.Response) bool {