Skip to content

Commit

Permalink
Add string-of-int lint, basically go vet 1.15
Browse files Browse the repository at this point in the history
This finds 21 failures, though only 3 of which are outside of tests.
In practice, these look like:
```go
# service/history/shard/controller.go:381:55: [string-of-int] dubious convertion of an integer into a string, use strconv.Itoa
info, err := c.GetHistoryServiceResolver().Lookup(string(shardID))
```
And they are *nearly always* a bug.

The correlation is so strong, `go test` on 1.15 and newer will by default refuse to compile a package with this code.
They have a pretty good bit on this in the release notes: https://golang.org/doc/go1.15#vet
To copy a relevant snippet from that:
> `string(9786)` does not evaluate to the string `"9786"`; it evaluates to the string `"\xe2\x98\xba"`, or `"☺"`.
So yeah.  That's basically never what is intended.

If you are on 1.15 or newer, you can reproduce these with `go vet -stringintconv ./...`.
Or just run `go test` on an affected package, it'll error rather than run tests.

We should *absolutely* prevent *all* of these.  We can't rely on `go` though, since this repo targets 1.12, so I'm glad this lint exists.

---

There is one spot I *did not* change (behaviorally), because I'm not sure what doing so will do.
The `shardIDstr` in service/frontend/adminHandler.go:251 is used as a hash-ring lookup key.
Changing this will change how shards hash, which could cause issues when deployed.

So, for now at least, I've converted it to the equivalent that the Go github issue recommends: `string(rune(int))`.
This produces the same values as before.

---

Strangely, this misses a fairly trivial case that `go vet` catches:
```go
--- a/service/frontend/adminHandler.go
+++ b/service/frontend/adminHandler.go
@@ -248,7 +248,7 @@ func (adh *adminHandlerImpl) DescribeWorkflowExecution(
        }

        shardID := common.WorkflowIDToHistoryShard(request.Execution.WorkflowID, adh.numberOfHistoryShards)
-       shardIDstr := string(shardID)             // misses this one
+	shardIDstr := string(rune(shardID)) // ...
        shardIDForOutput := strconv.Itoa(shardID) // correct, not changed in this diff

        historyHost, err := adh.GetMembershipMonitor().Lookup(common.HistoryServiceName, shardIDstr)
```
This might be due to the variable being handled correctly once?  I dunno.
I haven't been able to reproduce this with a smaller test, unfortunately.
Thankfully, this repo is open source, so it can serve as the reproduction.
  • Loading branch information
Groxx committed Feb 2, 2021
1 parent c929c39 commit beb64a7
Show file tree
Hide file tree
Showing 6 changed files with 29 additions and 22 deletions.
3 changes: 2 additions & 1 deletion client/history/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ package history

import (
"context"
"strconv"
"sync"
"time"

Expand Down Expand Up @@ -1048,7 +1049,7 @@ func (c *clientImpl) getClientForDomainID(domainID string) (Client, error) {
}

func (c *clientImpl) getClientForShardID(shardID int) (Client, error) {
client, err := c.clients.GetClientForKey(string(shardID))
client, err := c.clients.GetClientForKey(strconv.Itoa(shardID))
if err != nil {
return nil, err
}
Expand Down
5 changes: 5 additions & 0 deletions revive.toml
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,8 @@ warningCode = 0
[rule.var-declaration]
[rule.var-naming]

#### higher value stuff

# string(int) is almost always a bug.
# go vet considers this a fatal error, but only in 1.15 or newer, and go.mod currently targets 1.13
[rule.string-of-int]
2 changes: 1 addition & 1 deletion service/frontend/adminHandler.go
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ func (adh *adminHandlerImpl) DescribeWorkflowExecution(
}

shardID := common.WorkflowIDToHistoryShard(request.Execution.WorkflowID, adh.numberOfHistoryShards)
shardIDstr := string(shardID)
shardIDstr := string(rune(shardID)) // originally `string(int_shard_id)`, but changing it will change the ring hashing
shardIDForOutput := strconv.Itoa(shardID)

historyHost, err := adh.GetMembershipMonitor().Lookup(common.HistoryServiceName, shardIDstr)
Expand Down
4 changes: 2 additions & 2 deletions service/history/shard/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,7 @@ func (c *controller) getOrCreateHistoryShardItem(shardID int) (*historyShardsIte
if c.isShuttingDown() || atomic.LoadInt32(&c.status) == common.DaemonStatusStopped {
return nil, fmt.Errorf("controller for host '%v' shutting down", c.GetHostInfo().Identity())
}
info, err := c.GetHistoryServiceResolver().Lookup(string(shardID))
info, err := c.GetHistoryServiceResolver().Lookup(fmt.Sprint(shardID))
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -378,7 +378,7 @@ func (c *controller) acquireShards() {
if c.isShuttingDown() {
return
}
info, err := c.GetHistoryServiceResolver().Lookup(string(shardID))
info, err := c.GetHistoryServiceResolver().Lookup(fmt.Sprint(shardID))
if err != nil {
c.logger.Error("Error looking up host for shardID", tag.Error(err), tag.OperationFailed, tag.ShardID(shardID))
} else {
Expand Down
28 changes: 14 additions & 14 deletions service/history/shard/controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ func (s *controllerSuite) TestAcquireShardSuccess() {
if hostID == 0 {
myShards = append(myShards, shardID)
s.mockHistoryEngine.EXPECT().Start().Return().Times(1)
s.mockServiceResolver.EXPECT().Lookup(string(shardID)).Return(s.hostInfo, nil).Times(2)
s.mockServiceResolver.EXPECT().Lookup(fmt.Sprint(shardID)).Return(s.hostInfo, nil).Times(2)
s.mockEngineFactory.EXPECT().CreateEngine(gomock.Any()).Return(s.mockHistoryEngine).Times(1)
s.mockShardManager.On("GetShard", mock.Anything, &persistence.GetShardRequest{ShardID: shardID}).Return(
&persistence.GetShardResponse{
Expand Down Expand Up @@ -160,7 +160,7 @@ func (s *controllerSuite) TestAcquireShardSuccess() {
}).Return(nil).Once()
} else {
ownerHost := fmt.Sprintf("test-acquire-shard-host-%v", hostID)
s.mockServiceResolver.EXPECT().Lookup(string(shardID)).Return(membership.NewHostInfo(ownerHost, nil), nil).Times(1)
s.mockServiceResolver.EXPECT().Lookup(fmt.Sprint(shardID)).Return(membership.NewHostInfo(ownerHost, nil), nil).Times(1)
}
}

Expand Down Expand Up @@ -195,7 +195,7 @@ func (s *controllerSuite) TestAcquireShardsConcurrently() {
if hostID == 0 {
myShards = append(myShards, shardID)
s.mockHistoryEngine.EXPECT().Start().Return().Times(1)
s.mockServiceResolver.EXPECT().Lookup(string(shardID)).Return(s.hostInfo, nil).Times(2)
s.mockServiceResolver.EXPECT().Lookup(fmt.Sprint(shardID)).Return(s.hostInfo, nil).Times(2)
s.mockEngineFactory.EXPECT().CreateEngine(gomock.Any()).Return(s.mockHistoryEngine).Times(1)
s.mockShardManager.On("GetShard", mock.Anything, &persistence.GetShardRequest{ShardID: shardID}).Return(
&persistence.GetShardResponse{
Expand Down Expand Up @@ -243,7 +243,7 @@ func (s *controllerSuite) TestAcquireShardsConcurrently() {
}).Return(nil).Once()
} else {
ownerHost := fmt.Sprintf("test-acquire-shard-host-%v", hostID)
s.mockServiceResolver.EXPECT().Lookup(string(shardID)).Return(membership.NewHostInfo(ownerHost, nil), nil).Times(1)
s.mockServiceResolver.EXPECT().Lookup(fmt.Sprint(shardID)).Return(membership.NewHostInfo(ownerHost, nil), nil).Times(1)
}
}

Expand All @@ -263,12 +263,12 @@ func (s *controllerSuite) TestAcquireShardLookupFailure() {
numShards := 2
s.config.NumberOfShards = numShards
for shardID := 0; shardID < numShards; shardID++ {
s.mockServiceResolver.EXPECT().Lookup(string(shardID)).Return(nil, errors.New("ring failure")).Times(1)
s.mockServiceResolver.EXPECT().Lookup(fmt.Sprint(shardID)).Return(nil, errors.New("ring failure")).Times(1)
}

s.shardController.acquireShards()
for shardID := 0; shardID < numShards; shardID++ {
s.mockServiceResolver.EXPECT().Lookup(string(shardID)).Return(nil, errors.New("ring failure")).Times(1)
s.mockServiceResolver.EXPECT().Lookup(fmt.Sprint(shardID)).Return(nil, errors.New("ring failure")).Times(1)
s.Nil(s.shardController.GetEngineForShard(shardID))
}
}
Expand All @@ -285,7 +285,7 @@ func (s *controllerSuite) TestAcquireShardRenewSuccess() {

for shardID := 0; shardID < numShards; shardID++ {
s.mockHistoryEngine.EXPECT().Start().Return().Times(1)
s.mockServiceResolver.EXPECT().Lookup(string(shardID)).Return(s.hostInfo, nil).Times(2)
s.mockServiceResolver.EXPECT().Lookup(fmt.Sprint(shardID)).Return(s.hostInfo, nil).Times(2)
s.mockEngineFactory.EXPECT().CreateEngine(gomock.Any()).Return(s.mockHistoryEngine).Times(1)
s.mockShardManager.On("GetShard", mock.Anything, &persistence.GetShardRequest{ShardID: shardID}).Return(
&persistence.GetShardResponse{
Expand Down Expand Up @@ -339,7 +339,7 @@ func (s *controllerSuite) TestAcquireShardRenewSuccess() {
s.shardController.acquireShards()

for shardID := 0; shardID < numShards; shardID++ {
s.mockServiceResolver.EXPECT().Lookup(string(shardID)).Return(s.hostInfo, nil).Times(1)
s.mockServiceResolver.EXPECT().Lookup(fmt.Sprint(shardID)).Return(s.hostInfo, nil).Times(1)
}
s.shardController.acquireShards()

Expand All @@ -360,7 +360,7 @@ func (s *controllerSuite) TestAcquireShardRenewLookupFailed() {

for shardID := 0; shardID < numShards; shardID++ {
s.mockHistoryEngine.EXPECT().Start().Return().Times(1)
s.mockServiceResolver.EXPECT().Lookup(string(shardID)).Return(s.hostInfo, nil).Times(2)
s.mockServiceResolver.EXPECT().Lookup(fmt.Sprint(shardID)).Return(s.hostInfo, nil).Times(2)
s.mockEngineFactory.EXPECT().CreateEngine(gomock.Any()).Return(s.mockHistoryEngine).Times(1)
s.mockShardManager.On("GetShard", mock.Anything, &persistence.GetShardRequest{ShardID: shardID}).Return(
&persistence.GetShardResponse{
Expand Down Expand Up @@ -414,7 +414,7 @@ func (s *controllerSuite) TestAcquireShardRenewLookupFailed() {
s.shardController.acquireShards()

for shardID := 0; shardID < numShards; shardID++ {
s.mockServiceResolver.EXPECT().Lookup(string(shardID)).Return(nil, errors.New("ring failure")).Times(1)
s.mockServiceResolver.EXPECT().Lookup(fmt.Sprint(shardID)).Return(nil, errors.New("ring failure")).Times(1)
}
s.shardController.acquireShards()

Expand Down Expand Up @@ -461,7 +461,7 @@ func (s *controllerSuite) TestHistoryEngineClosed() {
for shardID := 0; shardID < 2; shardID++ {
mockEngine := historyEngines[shardID]
mockEngine.EXPECT().Stop().Return().Times(1)
s.mockServiceResolver.EXPECT().Lookup(string(shardID)).Return(differentHostInfo, nil).AnyTimes()
s.mockServiceResolver.EXPECT().Lookup(fmt.Sprint(shardID)).Return(differentHostInfo, nil).AnyTimes()
s.shardController.shardClosedCallback(shardID, nil)
}

Expand Down Expand Up @@ -506,7 +506,7 @@ func (s *controllerSuite) TestHistoryEngineClosed() {
for shardID := 2; shardID < numShards; shardID++ {
mockEngine := historyEngines[shardID]
mockEngine.EXPECT().Stop().Return().Times(1)
s.mockServiceResolver.EXPECT().Lookup(string(shardID)).Return(s.hostInfo, nil).AnyTimes()
s.mockServiceResolver.EXPECT().Lookup(fmt.Sprint(shardID)).Return(s.hostInfo, nil).AnyTimes()
}
s.shardController.Stop()
}
Expand Down Expand Up @@ -553,7 +553,7 @@ func (s *controllerSuite) TestShardControllerClosed() {
for shardID := 0; shardID < numShards; shardID++ {
mockEngine := historyEngines[shardID]
mockEngine.EXPECT().Stop().Times(1)
s.mockServiceResolver.EXPECT().Lookup(string(shardID)).Return(s.hostInfo, nil).AnyTimes()
s.mockServiceResolver.EXPECT().Lookup(fmt.Sprint(shardID)).Return(s.hostInfo, nil).AnyTimes()
}
s.shardController.Stop()
workerWG.Wait()
Expand All @@ -570,7 +570,7 @@ func (s *controllerSuite) setupMocksForAcquireShard(shardID int, mockEngine *eng

// s.mockResource.ExecutionMgr.On("Close").Return()
mockEngine.EXPECT().Start().Times(1)
s.mockServiceResolver.EXPECT().Lookup(string(shardID)).Return(s.hostInfo, nil).Times(2)
s.mockServiceResolver.EXPECT().Lookup(fmt.Sprint(shardID)).Return(s.hostInfo, nil).Times(2)
s.mockEngineFactory.EXPECT().CreateEngine(gomock.Any()).Return(mockEngine).Times(1)
s.mockShardManager.On("GetShard", mock.Anything, &persistence.GetShardRequest{ShardID: shardID}).Return(
&persistence.GetShardResponse{
Expand Down
9 changes: 5 additions & 4 deletions service/history/task/transfer_active_task_executor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ package task
import (
"context"
"math/rand"
"strconv"
"testing"
"time"

Expand Down Expand Up @@ -942,7 +943,7 @@ func (s *transferActiveTaskExecutorSuite) TestProcessCloseExecution_NoParent_Has
decisions = append(decisions, &types.Decision{
DecisionType: &dt,
StartChildWorkflowExecutionDecisionAttributes: &types.StartChildWorkflowExecutionDecisionAttributes{
WorkflowID: "child workflow" + string(i),
WorkflowID: "child workflow" + strconv.Itoa(i),
WorkflowType: &types.WorkflowType{
Name: "child workflow type",
},
Expand All @@ -961,7 +962,7 @@ func (s *transferActiveTaskExecutorSuite) TestProcessCloseExecution_NoParent_Has

for i := 0; i < 10; i++ {
_, _, err = mutableState.AddStartChildWorkflowExecutionInitiatedEvent(event.GetEventID(), uuid.New(), &types.StartChildWorkflowExecutionDecisionAttributes{
WorkflowID: "child workflow" + string(i),
WorkflowID: "child workflow" + strconv.Itoa(i),
WorkflowType: &types.WorkflowType{
Name: "child workflow type",
},
Expand Down Expand Up @@ -1039,7 +1040,7 @@ func (s *transferActiveTaskExecutorSuite) TestProcessCloseExecution_NoParent_Has
decisions = append(decisions, &types.Decision{
DecisionType: &dt,
StartChildWorkflowExecutionDecisionAttributes: &types.StartChildWorkflowExecutionDecisionAttributes{
WorkflowID: "child workflow" + string(i),
WorkflowID: "child workflow" + strconv.Itoa(i),
WorkflowType: &types.WorkflowType{
Name: "child workflow type",
},
Expand All @@ -1058,7 +1059,7 @@ func (s *transferActiveTaskExecutorSuite) TestProcessCloseExecution_NoParent_Has

for i := 0; i < 10; i++ {
_, _, err = mutableState.AddStartChildWorkflowExecutionInitiatedEvent(event.GetEventID(), uuid.New(), &types.StartChildWorkflowExecutionDecisionAttributes{
WorkflowID: "child workflow" + string(i),
WorkflowID: "child workflow" + strconv.Itoa(i),
WorkflowType: &types.WorkflowType{
Name: "child workflow type",
},
Expand Down

0 comments on commit beb64a7

Please sign in to comment.