Skip to content

Commit

Permalink
fix(GODT-1585): Fix data race on Session.name
Browse files Browse the repository at this point in the history
Remove Session.name as it is not worth the performance hit of
safeguarding this information for the use with logging.
  • Loading branch information
LBeernaertProton authored and jameshoulahan committed Aug 26, 2022
1 parent 9fd48f4 commit 6945147
Show file tree
Hide file tree
Showing 7 changed files with 9 additions and 39 deletions.
2 changes: 0 additions & 2 deletions internal/session/handle_close.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,6 @@ func (s *Session) handleClose(ctx context.Context, tag string, cmd *proto.Close,
return err
}

s.name = ""

ch <- response.Ok(tag).WithMessage("CLOSE")

return nil
Expand Down
2 changes: 0 additions & 2 deletions internal/session/handle_examine.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,6 @@ func (s *Session) handleExamine(ctx context.Context, tag string, cmd *proto.Exam
ch <- response.Ok().WithItems(response.ItemUnseen(unseen[0]))
}

s.name = mailbox.Name()

return nil
}); err != nil {
return err
Expand Down
2 changes: 0 additions & 2 deletions internal/session/handle_select.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,6 @@ func (s *Session) handleSelect(ctx context.Context, tag string, cmd *proto.Selec
ch <- response.Ok().WithItems(response.ItemUnseen(unseen[0])).WithMessage("Unseen messages")
}

s.name = mailbox.Name()

return nil
}); err != nil {
return err
Expand Down
2 changes: 0 additions & 2 deletions internal/session/handle_unselect.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@ func (s *Session) handleUnselect(ctx context.Context, tag string, cmd *proto.Uns
return err
}

s.name = ""

ch <- response.Ok(tag).WithMessage("UNSELECT")

return nil
Expand Down
8 changes: 2 additions & 6 deletions internal/session/logger.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,14 @@ import (
"strings"
)

func writeLog(w io.Writer, leader, sessionID, mailbox, line string) {
func writeLog(w io.Writer, leader, sessionID, line string) {
line = strings.ReplaceAll(strings.ReplaceAll(strings.ReplaceAll(strings.TrimSpace(line), "\r", `\r`), "\n", `\n`), "\t", `\t`)

if len(mailbox) > maxNameLength {
mailbox = mailbox[:maxNameLength] + "..."
}

if len(line) > maxLineLength {
line = line[:maxLineLength] + "..."
}

if _, err := fmt.Fprintf(w, "%v[%v][%v]: %v\n", leader, sessionID, mailbox, line); err != nil {
if _, err := fmt.Fprintf(w, "%v[%v]: %v\n", leader, sessionID, line); err != nil {
panic(err)
}
}
23 changes: 2 additions & 21 deletions internal/session/session.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,6 @@ type Session struct {
// state manages state of the authorized backend for this session.
state *backend.State

// name holds the name of the currently selected mailbox, if any.
name string

// userLock protects the session's user object.
userLock sync.Mutex

Expand Down Expand Up @@ -202,31 +199,15 @@ func (s *Session) logIncoming(line string) {
return
}

var name string

if s.name != "" {
name = s.name
} else {
name = "--"
}

writeLog(s.inLogger, "C", strconv.Itoa(s.sessionID), name, line)
writeLog(s.inLogger, "C", strconv.Itoa(s.sessionID), line)
}

func (s *Session) logOutgoing(line string) {
if s.outLogger == nil {
return
}

var name string

if s.name != "" {
name = s.name
} else {
name = "--"
}

writeLog(s.outLogger, "S", strconv.Itoa(s.sessionID), name, line)
writeLog(s.outLogger, "S", strconv.Itoa(s.sessionID), line)
}

func (s *Session) done(ctx context.Context) {
Expand Down
9 changes: 5 additions & 4 deletions tests/full_state_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"fmt"
"runtime/pprof"
"sync/atomic"
"testing"
"time"

Expand Down Expand Up @@ -99,19 +100,19 @@ func TestReceptionOnIdle(t *testing.T) {
require.Equal(t, uint32(0), status.Messages, "Expected message count does not match")

// prepare to stop idling.
stopped := false
stopped := int32(0)
stop := make(chan struct{})
done := make(chan error, 1)
// Create a channel to receive mailbox updates.
updates := make(chan client.Update)
updates := make(chan client.Update, 100)
c.Updates = updates

// idling.
go func() {
labels := pprof.Labels("test", "client", "idling", "idle")
pprof.Do(context.Background(), labels, func(_ context.Context) {
defer func() {
stopped = true
atomic.StoreInt32(&stopped, 1)
}()
done <- c.Idle(stop, nil)
})
Expand All @@ -137,7 +138,7 @@ func TestReceptionOnIdle(t *testing.T) {
var existsUpdate uint32 = 0
var recentUpdate uint32 = 0

for !stopped {
for atomic.LoadInt32(&stopped) == 0 {
select {
case update := <-updates:
boxUpdate, ok := update.(*client.MailboxUpdate)
Expand Down

0 comments on commit 6945147

Please sign in to comment.