From f6373f7a4d1d68a7380ecb91d497fbf0f77e4727 Mon Sep 17 00:00:00 2001 From: Shivaram Lingamneni Date: Fri, 4 May 2018 00:24:54 -0400 Subject: [PATCH] fix #262 --- irc/client.go | 2 +- irc/getters.go | 13 ++++++ irc/handlers.go | 14 +++--- irc/nickname.go | 6 +-- irc/whowas.go | 105 ++++++++++++++++++++++----------------------- irc/whowas_test.go | 102 +++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 179 insertions(+), 63 deletions(-) create mode 100644 irc/whowas_test.go diff --git a/irc/client.go b/irc/client.go index ef15e237..30776416 100644 --- a/irc/client.go +++ b/irc/client.go @@ -690,7 +690,7 @@ func (client *Client) destroy(beingResumed bool) { client.Quit("Connection closed") if !beingResumed { - client.server.whoWas.Append(client) + client.server.whoWas.Append(client.WhoWas()) } // remove from connection limits diff --git a/irc/getters.go b/irc/getters.go index 24a0455a..6714592d 100644 --- a/irc/getters.go +++ b/irc/getters.go @@ -209,6 +209,19 @@ func (client *Client) Channels() (result []*Channel) { return } +func (client *Client) WhoWas() (result WhoWas) { + client.stateMutex.RLock() + defer client.stateMutex.RUnlock() + + result.nicknameCasefolded = client.nickCasefolded + result.nickname = client.nick + result.username = client.username + result.hostname = client.hostname + result.realname = client.realname + + return +} + func (channel *Channel) Name() string { channel.stateMutex.RLock() defer channel.stateMutex.RUnlock() diff --git a/irc/handlers.go b/irc/handlers.go index bcde94eb..52a68495 100644 --- a/irc/handlers.go +++ b/irc/handlers.go @@ -2507,27 +2507,29 @@ func whoisHandler(server *Server, client *Client, msg ircmsg.IrcMessage, rb *Res func whowasHandler(server *Server, client *Client, msg ircmsg.IrcMessage, rb *ResponseBuffer) bool { nicknames := strings.Split(msg.Params[0], ",") - var count int64 + // 0 means "all the entries", as does a negative number + var count uint64 if len(msg.Params) > 1 { - count, _ = strconv.ParseInt(msg.Params[1], 10, 64) + count, _ = strconv.ParseUint(msg.Params[1], 10, 64) } //var target string //if len(msg.Params) > 2 { // target = msg.Params[2] //} + cnick := client.Nick() for _, nickname := range nicknames { - results := server.whoWas.Find(nickname, count) + results := server.whoWas.Find(nickname, int(count)) if len(results) == 0 { if len(nickname) > 0 { - rb.Add(nil, server.name, ERR_WASNOSUCHNICK, client.nick, nickname, client.t("There was no such nickname")) + rb.Add(nil, server.name, ERR_WASNOSUCHNICK, cnick, nickname, client.t("There was no such nickname")) } } else { for _, whoWas := range results { - rb.Add(nil, server.name, RPL_WHOWASUSER, client.nick, whoWas.nickname, whoWas.username, whoWas.hostname, "*", whoWas.realname) + rb.Add(nil, server.name, RPL_WHOWASUSER, cnick, whoWas.nickname, whoWas.username, whoWas.hostname, "*", whoWas.realname) } } if len(nickname) > 0 { - rb.Add(nil, server.name, RPL_ENDOFWHOWAS, client.nick, nickname, client.t("End of WHOWAS")) + rb.Add(nil, server.name, RPL_ENDOFWHOWAS, cnick, nickname, client.t("End of WHOWAS")) } } return false diff --git a/irc/nickname.go b/irc/nickname.go index 4251e387..75987990 100644 --- a/irc/nickname.go +++ b/irc/nickname.go @@ -43,8 +43,8 @@ func performNickChange(server *Server, client *Client, target *Client, newnick s } hadNick := target.HasNick() - origNick := target.Nick() origNickMask := target.NickMaskString() + whowas := client.WhoWas() err = client.server.clients.SetNick(target, nickname) if err == errNicknameInUse { rb.Add(nil, server.name, ERR_NICKNAMEINUSE, client.nick, nickname, client.t("Nickname is already in use")) @@ -61,8 +61,8 @@ func performNickChange(server *Server, client *Client, target *Client, newnick s client.server.logger.Debug("nick", fmt.Sprintf("%s changed nickname to %s [%s]", origNickMask, nickname, cfnick)) if hadNick { - target.server.snomasks.Send(sno.LocalNicks, fmt.Sprintf(ircfmt.Unescape("$%s$r changed nickname to %s"), origNick, nickname)) - target.server.whoWas.Append(client) + target.server.snomasks.Send(sno.LocalNicks, fmt.Sprintf(ircfmt.Unescape("$%s$r changed nickname to %s"), whowas.nickname, nickname)) + target.server.whoWas.Append(whowas) for friend := range target.Friends() { friend.Send(nil, origNickMask, "NICK", nickname) } diff --git a/irc/whowas.go b/irc/whowas.go index 7a55da5c..7ad3756f 100644 --- a/irc/whowas.go +++ b/irc/whowas.go @@ -10,11 +10,16 @@ import ( // WhoWasList holds our list of prior clients (for use with the WHOWAS command). type WhoWasList struct { - buffer []*WhoWas - start int - end int + buffer []WhoWas + // three possible states: + // empty: start == end == -1 + // partially full: start != end + // full: start == end > 0 + // if entries exist, they go from `start` to `(end - 1) % length` + start int + end int - accessMutex sync.RWMutex // tier 2 + accessMutex sync.RWMutex // tier 1 } // WhoWas is an entry in the WhoWasList. @@ -29,77 +34,71 @@ type WhoWas struct { // NewWhoWasList returns a new WhoWasList func NewWhoWasList(size uint) *WhoWasList { return &WhoWasList{ - buffer: make([]*WhoWas, size+1), + buffer: make([]WhoWas, size), + start: -1, + end: -1, } } // Append adds an entry to the WhoWasList. -func (list *WhoWasList) Append(client *Client) { +func (list *WhoWasList) Append(whowas WhoWas) { list.accessMutex.Lock() defer list.accessMutex.Unlock() - list.buffer[list.end] = &WhoWas{ - nicknameCasefolded: client.nickCasefolded, - nickname: client.nick, - username: client.username, - hostname: client.hostname, - realname: client.realname, + if len(list.buffer) == 0 { + return } - list.end = (list.end + 1) % len(list.buffer) - if list.end == list.start { - list.start = (list.end + 1) % len(list.buffer) + + var pos int + if list.start == -1 { // empty + pos = 0 + list.start = 0 + list.end = 1 + } else if list.start != list.end { // partially full + pos = list.end + list.end = (list.end + 1) % len(list.buffer) + } else if list.start == list.end { // full + pos = list.end + list.end = (list.end + 1) % len(list.buffer) + list.start = list.end // advance start as well, overwriting first entry } + + list.buffer[pos] = whowas } // Find tries to find an entry in our WhoWasList with the given details. -func (list *WhoWasList) Find(nickname string, limit int64) []*WhoWas { +func (list *WhoWasList) Find(nickname string, limit int) (results []WhoWas) { + casefoldedNickname, err := CasefoldName(nickname) + if err != nil { + return + } + list.accessMutex.RLock() defer list.accessMutex.RUnlock() - results := make([]*WhoWas, 0) - - casefoldedNickname, err := CasefoldName(nickname) - if err != nil { - return results + if list.start == -1 { + return } - - for whoWas := range list.Each() { - if casefoldedNickname != whoWas.nicknameCasefolded { - continue + // iterate backwards through the ring buffer + pos := list.prev(list.end) + for limit == 0 || len(results) < limit { + if casefoldedNickname == list.buffer[pos].nicknameCasefolded { + results = append(results, list.buffer[pos]) } - results = append(results, whoWas) - if int64(len(results)) >= limit { + if pos == list.start { break } + pos = list.prev(pos) } - return results + + return } func (list *WhoWasList) prev(index int) int { - list.accessMutex.RLock() - defer list.accessMutex.RUnlock() - - index-- - if index < 0 { - index += len(list.buffer) + switch index { + case 0: + return len(list.buffer) - 1 + default: + return index - 1 } - return index -} - -// Each iterates the WhoWasList in reverse. -func (list *WhoWasList) Each() <-chan *WhoWas { - ch := make(chan *WhoWas) - go func() { - defer close(ch) - if list.start == list.end { - return - } - start := list.prev(list.end) - end := list.prev(list.start) - for start != end { - ch <- list.buffer[start] - start = list.prev(start) - } - }() - return ch } diff --git a/irc/whowas_test.go b/irc/whowas_test.go new file mode 100644 index 00000000..82eaa231 --- /dev/null +++ b/irc/whowas_test.go @@ -0,0 +1,102 @@ +// Copyright (c) 2018 Shivaram Lingamneni +// released under the MIT license + +package irc + +import ( + "testing" +) + +func makeTestWhowas(nick string) WhoWas { + cfnick, err := CasefoldName(nick) + if err != nil { + panic(err) + } + return WhoWas{ + nicknameCasefolded: cfnick, + nickname: nick, + username: "user", + hostname: "oragono.io", + realname: "Real Name", + } +} + +func TestWhoWas(t *testing.T) { + var results []WhoWas + wwl := NewWhoWasList(3) + // test Find on empty list + results = wwl.Find("nobody", 10) + if len(results) != 0 { + t.Fatalf("incorrect whowas results: %v", results) + } + + wwl.Append(makeTestWhowas("dan-")) + results = wwl.Find("nobody", 10) + if len(results) != 0 { + t.Fatalf("incorrect whowas results: %v", results) + } + results = wwl.Find("dan-", 10) + if len(results) != 1 || results[0].nickname != "dan-" { + t.Fatalf("incorrect whowas results: %v", results) + } + + wwl.Append(makeTestWhowas("slingamn")) + results = wwl.Find("slingamN", 10) + if len(results) != 1 || results[0].nickname != "slingamn" { + t.Fatalf("incorrect whowas results: %v", results) + } + + wwl.Append(makeTestWhowas("Dan-")) + results = wwl.Find("dan-", 10) + // reverse chronological order + if len(results) != 2 || results[0].nickname != "Dan-" || results[1].nickname != "dan-" { + t.Fatalf("incorrect whowas results: %v", results) + } + // 0 means no limit + results = wwl.Find("dan-", 0) + if len(results) != 2 || results[0].nickname != "Dan-" || results[1].nickname != "dan-" { + t.Fatalf("incorrect whowas results: %v", results) + } + // a limit of 1 should return the most recent entry only + results = wwl.Find("dan-", 1) + if len(results) != 1 || results[0].nickname != "Dan-" { + t.Fatalf("incorrect whowas results: %v", results) + } + + wwl.Append(makeTestWhowas("moocow")) + results = wwl.Find("moocow", 10) + if len(results) != 1 || results[0].nickname != "moocow" { + t.Fatalf("incorrect whowas results: %v", results) + } + results = wwl.Find("dan-", 10) + // should have overwritten the original entry, leaving the second + if len(results) != 1 || results[0].nickname != "Dan-" { + t.Fatalf("incorrect whowas results: %v", results) + } + + // overwrite the second entry + wwl.Append(makeTestWhowas("enckse")) + results = wwl.Find("enckse", 10) + if len(results) != 1 || results[0].nickname != "enckse" { + t.Fatalf("incorrect whowas results: %v", results) + } + results = wwl.Find("slingamn", 10) + if len(results) != 0 { + t.Fatalf("incorrect whowas results: %v", results) + } +} + + +func TestEmptyWhoWas(t *testing.T) { + // stupid edge case; setting an empty whowas buffer should not panic + wwl := NewWhoWasList(0) + results := wwl.Find("slingamn", 10) + if len(results) != 0 { + t.Fatalf("incorrect whowas results: %v", results) + } + wwl.Append(makeTestWhowas("slingamn")) + results = wwl.Find("slingamn", 10) + if len(results) != 0 { + t.Fatalf("incorrect whowas results: %v", results) + } +}