yesBad/proton-bridge
1
0
Fork 0
forked from Silverfish/proton-bridge
Code Actions Activity

fix(GODT-2500): Recover in deferred function.

Browse Source
This commit is contained in:
Jakub
2023-04-04 15:27:03 +02:00
parent fd100eecc2
commit 7330406752
18 changed files with 103 additions and 85 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
internal/app/app.go
View File

@ -28,6 +28,7 @@ import (
"time"
"github.com/Masterminds/semver/v3"
"github.com/ProtonMail/gluon/async"
"github.com/ProtonMail/proton-bridge/v3/internal/bridge"
"github.com/ProtonMail/proton-bridge/v3/internal/constants"
"github.com/ProtonMail/proton-bridge/v3/internal/cookies"
@ -379,7 +380,7 @@ func withCrashHandler(restarter *restarter.Restarter, reporter *sentry.Reporter,
defer logrus.Debug("Crash handler stopped")
crashHandler := crash.NewHandler(crash.ShowErrorNotification(constants.FullAppName))
defer crashHandler.HandlePanic()
defer async.HandlePanic(crashHandler)
// On crash, send crash report to Sentry.
crashHandler.AddRecoveryAction(reporter.ReportException)

5
internal/app/frontend.go
View File

@ -46,10 +46,11 @@ func runFrontend(
switch {
case c.Bool(flagCLI):
return bridgeCLI.New(bridge, restarter, eventCh, crashHandler).Loop()
return bridgeCLI.New(bridge, restarter, eventCh, crashHandler, quitCh).Loop()
case c.Bool(flagNonInteractive):
select {}
<-quitCh
return nil
case c.Bool(flagGRPC):
service, err := grpc.NewService(crashHandler, restarter, locations, bridge, eventCh, quitCh, !c.Bool(flagNoWindow), parentPID)

2
internal/app/singleinstance.go
View File

@ -40,7 +40,7 @@ func checkSingleInstance(settingPath, lockFilePath string, curVersion *semver.Ve
return lock, nil
}
logrus.Debug("Failed to create lock file; another instance is running")
logrus.Warn("Failed to create lock file; another instance is running")
// We couldn't create the lock file, so another instance is probably running.
// Check if it's an older version of the app.

2
internal/bridge/mocks.go
View File

@ -45,7 +45,7 @@ func NewMocks(tb testing.TB, version, minAuto *semver.Version) *Mocks {
mocks.TLSReporter.EXPECT().GetTLSIssueCh().Return(mocks.TLSIssueCh).AnyTimes()
// This is called at he end of any go-routine:
mocks.CrashHandler.EXPECT().HandlePanic().AnyTimes()
mocks.CrashHandler.EXPECT().HandlePanic(gomock.Any()).AnyTimes()
return mocks
}

10
internal/bridge/mocks/async_mocks.go
View File

@ -1,5 +1,5 @@
// Code generated by MockGen. DO NOT EDIT.
// Source: github.com/ProtonMail/proton-bridge/v3/internal/async (interfaces: PanicHandler)
// Source: github.com/ProtonMail/gluon/async (interfaces: PanicHandler)
// Package mocks is a generated GoMock package.
package mocks
@ -34,13 +34,13 @@ func (m *MockPanicHandler) EXPECT() *MockPanicHandlerMockRecorder {
}
// HandlePanic mocks base method.
func (m *MockPanicHandler) HandlePanic() {
func (m *MockPanicHandler) HandlePanic(arg0 interface{}) {
m.ctrl.T.Helper()
m.ctrl.Call(m, "HandlePanic")
m.ctrl.Call(m, "HandlePanic", arg0)
}
// HandlePanic indicates an expected call of HandlePanic.
func (mr *MockPanicHandlerMockRecorder) HandlePanic() *gomock.Call {
func (mr *MockPanicHandlerMockRecorder) HandlePanic(arg0 interface{}) *gomock.Call {
mr.mock.ctrl.T.Helper()
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "HandlePanic", reflect.TypeOf((*MockPanicHandler)(nil).HandlePanic))
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "HandlePanic", reflect.TypeOf((*MockPanicHandler)(nil).HandlePanic), arg0)
}

14
internal/crash/handler.go
View File

@ -38,14 +38,16 @@ func (h *Handler) AddRecoveryAction(action RecoveryAction) *Handler {
return h
}
func (h *Handler) HandlePanic() {
func (h *Handler) HandlePanic(r interface{}) {
sentry.SkipDuringUnwind()
if r := recover(); r != nil {
for _, action := range h.actions {
if err := action(r); err != nil {
logrus.WithError(err).Error("Failed to execute recovery action")
}
if r == nil {
return
}
for _, action := range h.actions {
if err := action(r); err != nil {
logrus.WithError(err).Error("Failed to execute recovery action")
}
}
}

53
internal/crash/handler_test.go
View File

@ -21,38 +21,41 @@ import (
"fmt"
"testing"
"github.com/ProtonMail/gluon/async"
"github.com/stretchr/testify/assert"
)
func TestHandler(t *testing.T) {
var s string
assert.NotPanics(t, func() {
var s string
h := NewHandler(
func(r interface{}) error {
s += fmt.Sprintf("1: %v\n", r)
return nil
},
func(r interface{}) error {
s += fmt.Sprintf("2: %v\n", r)
return nil
},
)
h := NewHandler(
func(r interface{}) error {
s += fmt.Sprintf("1: %v\n", r)
return nil
},
func(r interface{}) error {
s += fmt.Sprintf("2: %v\n", r)
return nil
},
)
h.
AddRecoveryAction(func(r interface{}) error {
s += fmt.Sprintf("3: %v\n", r)
return nil
}).
AddRecoveryAction(func(r interface{}) error {
s += fmt.Sprintf("4: %v\n", r)
return nil
})
h.
AddRecoveryAction(func(r interface{}) error {
s += fmt.Sprintf("3: %v\n", r)
return nil
}).
AddRecoveryAction(func(r interface{}) error {
s += fmt.Sprintf("4: %v\n", r)
return nil
})
defer func() {
assert.Equal(t, "1: thing\n2: thing\n3: thing\n4: thing\n", s)
}()
defer func() {
assert.Equal(t, "1: thing\n2: thing\n3: thing\n4: thing\n", s)
}()
defer h.HandlePanic()
defer async.HandlePanic(h)
panic("thing")
panic("thing")
})
}

13
internal/frontend/cli/frontend.go
View File

@ -45,7 +45,13 @@ type frontendCLI struct {
}
// New returns a new CLI frontend configured with the given options.
func New(bridge *bridge.Bridge, restarter *restarter.Restarter, eventCh <-chan events.Event, panicHandler async.PanicHandler) *frontendCLI { //nolint:revive
func New(
bridge *bridge.Bridge,
restarter *restarter.Restarter,
eventCh <-chan events.Event,
panicHandler async.PanicHandler,
quitCh <-chan struct{},
) *frontendCLI { //nolint:revive
fe := &frontendCLI{
Shell: ishell.New(),
bridge: bridge,
@ -285,6 +291,11 @@ func New(bridge *bridge.Bridge, restarter *restarter.Restarter, eventCh <-chan e
go fe.watchEvents(eventCh)
go func() {
<-quitCh
fe.Close()
}()
return fe
}

16
internal/frontend/grpc/service.go
View File

@ -70,7 +70,7 @@ type Service struct { // nolint:structcheck
eventQueue []*StreamEvent
eventQueueMutex sync.Mutex
panicHandler CrashHandler
panicHandler async.PanicHandler
restarter Restarter
bridge *bridge.Bridge
eventCh <-chan events.Event
@ -97,7 +97,7 @@ type Service struct { // nolint:structcheck
// NewService returns a new instance of the service.
func NewService(
panicHandler CrashHandler,
panicHandler async.PanicHandler,
restarter Restarter,
locations service.Locator,
bridge *bridge.Bridge,
@ -192,10 +192,6 @@ func NewService(
return s, nil
}
func (s *Service) handlePanic() {
async.HandlePanic(s.panicHandler)
}
func (s *Service) initAutostart() {
s.firstTimeAutostart.Do(func() {
shouldAutostartBeOn := s.bridge.GetAutostart()
@ -213,13 +209,13 @@ func (s *Service) Loop() error {
s.log.Info("Not monitoring parent PID")
} else {
go func() {
defer s.handlePanic()
defer async.HandlePanic(s.panicHandler)
s.monitorParentPID()
}()
}
go func() {
defer s.handlePanic()
defer async.HandlePanic(s.panicHandler)
s.watchEvents()
}()
@ -229,7 +225,7 @@ func (s *Service) Loop() error {
defer close(doneCh)
go func() {
defer s.handlePanic()
defer async.HandlePanic(s.panicHandler)
select {
case <-s.quitCh:
@ -577,7 +573,7 @@ func (s *Service) monitorParentPID() {
s.log.Info("Parent process does not exist anymore. Initiating shutdown")
// quit will write to the parentPIDDoneCh, so we launch a goroutine.
go func() {
defer s.handlePanic()
defer async.HandlePanic(s.panicHandler)
if err := s.quit(); err != nil {
logrus.WithError(err).Error("Error on quit")

25
internal/frontend/grpc/service_methods.go
View File

@ -26,6 +26,7 @@ import (
"runtime"
"github.com/Masterminds/semver/v3"
"github.com/ProtonMail/gluon/async"
"github.com/ProtonMail/go-proton-api"
"github.com/ProtonMail/proton-bridge/v3/internal/bridge"
"github.com/ProtonMail/proton-bridge/v3/internal/constants"
@ -114,7 +115,7 @@ func (s *Service) Quit(ctx context.Context, empty *emptypb.Empty) (*emptypb.Empt
func (s *Service) quit() error {
// Windows is notably slow at Quitting. We do it in a goroutine to speed things up a bit.
go func() {
defer s.handlePanic()
defer async.HandlePanic(s.panicHandler)
if s.parentPID >= 0 {
s.parentPIDDoneCh <- struct{}{}
@ -223,7 +224,7 @@ func (s *Service) TriggerReset(ctx context.Context, _ *emptypb.Empty) (*emptypb.
s.log.Debug("TriggerReset")
go func() {
defer s.handlePanic()
defer async.HandlePanic(s.panicHandler)
s.triggerReset()
}()
@ -319,7 +320,7 @@ func (s *Service) ReportBug(ctx context.Context, report *ReportBugRequest) (*emp
}).Debug("ReportBug")
go func() {
defer s.handlePanic()
defer async.HandlePanic(s.panicHandler)
defer func() { _ = s.SendEvent(NewReportBugFinishedEvent()) }()
@ -348,7 +349,7 @@ func (s *Service) ExportTLSCertificates(_ context.Context, folderPath *wrappersp
s.log.WithField("folderPath", folderPath).Info("ExportTLSCertificates")
go func() {
defer s.handlePanic()
defer async.HandlePanic(s.panicHandler)
cert, key := s.bridge.GetBridgeTLSCert()
@ -384,7 +385,7 @@ func (s *Service) Login(ctx context.Context, login *LoginRequest) (*emptypb.Empt
s.log.WithField("username", login.Username).Debug("Login")
go func() {
defer s.handlePanic()
defer async.HandlePanic(s.panicHandler)
password, err := base64Decode(login.Password)
if err != nil {
@ -440,7 +441,7 @@ func (s *Service) Login2FA(ctx context.Context, login *LoginRequest) (*emptypb.E
s.log.WithField("username", login.Username).Debug("Login2FA")
go func() {
defer s.handlePanic()
defer async.HandlePanic(s.panicHandler)
if s.auth.UID == "" || s.authClient == nil {
s.log.Errorf("Login 2FA: authethication incomplete %s %p", s.auth.UID, s.authClient)
@ -485,7 +486,7 @@ func (s *Service) Login2Passwords(ctx context.Context, login *LoginRequest) (*em
s.log.WithField("username", login.Username).Debug("Login2Passwords")
go func() {
defer s.handlePanic()
defer async.HandlePanic(s.panicHandler)
password, err := base64Decode(login.Password)
if err != nil {
@ -507,7 +508,7 @@ func (s *Service) LoginAbort(ctx context.Context, loginAbort *LoginAbortRequest)
s.log.WithField("username", loginAbort.Username).Debug("LoginAbort")
go func() {
defer s.handlePanic()
defer async.HandlePanic(s.panicHandler)
s.loginAbort()
}()
@ -519,7 +520,7 @@ func (s *Service) CheckUpdate(context.Context, *emptypb.Empty) (*emptypb.Empty,
s.log.Debug("CheckUpdate")
go func() {
defer s.handlePanic()
defer async.HandlePanic(s.panicHandler)
updateCh, done := s.bridge.GetEvents(
events.UpdateAvailable{},
@ -551,7 +552,7 @@ func (s *Service) InstallUpdate(ctx context.Context, _ *emptypb.Empty) (*emptypb
s.log.Debug("InstallUpdate")
go func() {
defer s.handlePanic()
defer async.HandlePanic(s.panicHandler)
safe.RLock(func() {
s.bridge.InstallUpdate(s.target)
@ -592,7 +593,7 @@ func (s *Service) SetDiskCachePath(ctx context.Context, newPath *wrapperspb.Stri
s.log.WithField("path", newPath.Value).Debug("setDiskCachePath")
go func() {
defer s.handlePanic()
defer async.HandlePanic(s.panicHandler)
defer func() {
_ = s.SendEvent(NewDiskCachePathChangeFinishedEvent())
@ -659,7 +660,7 @@ func (s *Service) SetMailServerSettings(_ context.Context, settings *ImapSmtpSet
Debug("SetConnectionMode")
go func() {
defer s.handlePanic()
defer async.HandlePanic(s.panicHandler)
defer func() { _ = s.SendEvent(NewChangeMailServerSettingsFinishedEvent()) }()

3
internal/frontend/grpc/service_stream.go
View File

@ -20,6 +20,7 @@ package grpc
import (
"context"
"github.com/ProtonMail/gluon/async"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
"google.golang.org/protobuf/types/known/emptypb"
@ -49,7 +50,7 @@ func (s *Service) RunEventStream(request *EventStreamRequest, server Bridge_RunE
// if events occurred before streaming started, they've been queued. Now that the stream channel is available
// we can flush the queued
go func() {
defer s.handlePanic()
defer async.HandlePanic(s.panicHandler)
s.eventQueueMutex.Lock()
defer s.eventQueueMutex.Unlock()

7
internal/frontend/grpc/service_user.go
View File

@ -20,6 +20,7 @@ package grpc
import (
"context"
"github.com/ProtonMail/gluon/async"
"github.com/ProtonMail/proton-bridge/v3/internal/vault"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
@ -70,7 +71,7 @@ func (s *Service) SetUserSplitMode(ctx context.Context, splitMode *UserSplitMode
}
go func() {
defer s.handlePanic()
defer async.HandlePanic(s.panicHandler)
defer func() { _ = s.SendEvent(NewUserToggleSplitModeFinishedEvent(splitMode.UserID)) }()
var targetMode vault.AddressMode
@ -121,7 +122,7 @@ func (s *Service) LogoutUser(ctx context.Context, userID *wrapperspb.StringValue
}
go func() {
defer s.handlePanic()
defer async.HandlePanic(s.panicHandler)
if err := s.bridge.LogoutUser(context.Background(), userID.Value); err != nil {
s.log.WithError(err).Error("Failed to log user out")
@ -135,7 +136,7 @@ func (s *Service) RemoveUser(ctx context.Context, userID *wrapperspb.StringValue
s.log.WithField("UserID", userID.Value).Debug("RemoveUser")
go func() {
defer s.handlePanic()
defer async.HandlePanic(s.panicHandler)
// remove preferences
if err := s.bridge.DeleteUser(context.Background(), userID.Value); err != nil {

4
internal/frontend/grpc/types.go
View File

@ -17,10 +17,6 @@
package grpc
type CrashHandler interface {
HandlePanic()
}
type Restarter interface {
Set(restart, crash bool)
AddFlags(flags ...string)

10
internal/user/sync.go
View File

@ -370,7 +370,7 @@ func (user *User) syncMessages(
errorCh := make(chan error, maxParallelDownloads*4)
// Go routine in charge of downloading message metadata
logging.GoAnnotated(ctx, user.panicHandler, func(ctx context.Context) {
async.GoAnnotated(ctx, user.panicHandler, func(ctx context.Context) {
defer close(downloadCh)
const MetadataDataPageSize = 150
@ -433,7 +433,7 @@ func (user *User) syncMessages(
}, logging.Labels{"sync-stage": "meta-data"})
// Goroutine in charge of downloading and building messages in maxBatchSize batches.
logging.GoAnnotated(ctx, user.panicHandler, func(ctx context.Context) {
async.GoAnnotated(ctx, user.panicHandler, func(ctx context.Context) {
defer close(buildCh)
defer close(errorCh)
defer func() {
@ -492,7 +492,7 @@ func (user *User) syncMessages(
}, logging.Labels{"sync-stage": "download"})
// Goroutine which builds messages after they have been downloaded
logging.GoAnnotated(ctx, user.panicHandler, func(ctx context.Context) {
async.GoAnnotated(ctx, user.panicHandler, func(ctx context.Context) {
defer close(flushCh)
defer func() {
logrus.Debugf("sync builder exit")
@ -530,7 +530,7 @@ func (user *User) syncMessages(
}, logging.Labels{"sync-stage": "builder"})
// Goroutine which converts the messages into updates and builds a waitable structure for progress tracking.
logging.GoAnnotated(ctx, user.panicHandler, func(ctx context.Context) {
async.GoAnnotated(ctx, user.panicHandler, func(ctx context.Context) {
defer close(flushUpdateCh)
defer func() {
logrus.Debugf("sync flush exit")
@ -780,7 +780,7 @@ func (user *User) newAttachmentDownloader(ctx context.Context, client *proton.Cl
ctx, cancel := context.WithCancel(ctx)
for i := 0; i < workerCount; i++ {
workerCh = make(chan attachmentJob)
logging.GoAnnotated(ctx, user.panicHandler, func(ctx context.Context) { attachmentWorker(ctx, client, workerCh) }, logging.Labels{
async.GoAnnotated(ctx, user.panicHandler, func(ctx context.Context) { attachmentWorker(ctx, client, workerCh) }, logging.Labels{
"sync": fmt.Sprintf("att-downloader %v", i),
})
}