Skip to content

Commit cc230a2

Browse files
committed
Merge branch 'dev'
2 parents a7ad9ec + dc0ef33 commit cc230a2

32 files changed

+1003
-192
lines changed

.gitignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,3 +17,4 @@ lint.log
1717
tchannel-go.iml
1818
.vscode
1919
.bin/
20+
.idea/

.travis.yml

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,19 +6,21 @@ cache:
66
- $HOME/.glide/cache
77

88
go:
9-
- 1.5
10-
- 1.6
119
- 1.7
1210
- 1.8
11+
- 1.9
1312

1413
matrix:
1514
include:
16-
- go: 1.6
15+
- go: 1.9
1716
env: CROSSDOCK=true
1817
sudo: required
1918
dist: trusty
2019
services:
2120
- docker
21+
include:
22+
- go: 1.9
23+
env: NO_TEST=yes COVERAGE=yes LINT=yes
2224

2325
env:
2426
global:
@@ -37,9 +39,9 @@ install:
3739
- make install_ci
3840

3941
script:
40-
- make test_ci
41-
- make cover_ci
42-
- make lint
42+
- test -n "$NO_TEST" || make test_ci
43+
- test -z "$COVERAGE" || make cover_ci
44+
- test -z "$LINT" || make install_lint lint
4345
- make crossdock_logs_ci
4446

4547
after_success:

CHANGELOG.md

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,15 @@
11
Changelog
22
=========
33

4+
# v1.8.0 (2017-11-06)
5+
6+
* Improve error logging on `thrift.Server` errors. (#663)
7+
* Reduce memory usage for idle connections. (#658)
8+
* Add opt-in active connection health checks. (#318)
9+
* Don't close connections on ping errors.(#655)
10+
* Unpin and reduce dependencies in `glide.yaml` by using `testImports`. (#649)
11+
* Avoid holding on to closed connections' memory in peers. (#644)
12+
413
# v1.7.0 (2017-08-04)
514

615
* Cancel the context on incoming calls if the client connection is closed.

Makefile

Lines changed: 8 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,4 @@
11
export GO15VENDOREXPERIMENT=1
2-
GO_VERSION := $(shell go version | awk '{ print $$3 }')
3-
GO_MINOR_VERSION := $(word 2,$(subst ., ,$(GO_VERSION)))
4-
LINTABLE_MINOR_VERSIONS := 6
5-
FMTABLE_MINOR_VERSIONS := 7
6-
ifneq ($(filter $(LINTABLE_MINOR_VERSIONS),$(GO_MINOR_VERSION)),)
7-
SHOULD_LINT := true
8-
endif
9-
ifneq ($(filter $(FMTABLE_MINOR_VERSIONS),$(GO_MINOR_VERSION)),)
10-
SHOULD_LINT_FMT := true
11-
endif
122

133
PATH := $(GOPATH)/bin:$(PATH)
144
EXAMPLES=./examples/bench/server ./examples/bench/client ./examples/ping ./examples/thrift ./examples/hyperbahn/echo-server
@@ -58,20 +48,15 @@ install:
5848
GOPATH=$(OLD_GOPATH) glide --debug install --cache --cache-gopath
5949

6050
install_lint:
61-
ifdef SHOULD_LINT
62-
@echo "Installing golint, since we expect to lint on" $(GO_VERSION)
51+
@echo "Installing golint, since we expect to lint"
6352
GOPATH=$(OLD_GOPATH) go get -u -f github.com/golang/lint/golint
64-
else
65-
@echo "Not installing golint, since we don't lint on" $(GO_VERSION)
66-
endif
6753

6854
install_glide:
6955
# all we want is: GOPATH=$(OLD_GOPATH) go get -u github.com/Masterminds/glide
7056
# but have to pin to 0.12.3 due to https://github.com/Masterminds/glide/issues/745
7157
GOPATH=$(OLD_GOPATH) go get -u github.com/Masterminds/glide && cd $(OLD_GOPATH)/src/github.com/Masterminds/glide && git checkout v0.12.3 && go install
7258

73-
install_ci: $(BIN)/thrift install_glide install_lint install
74-
GOPATH=$(OLD_GOPATH) go get -u github.com/mattn/goveralls
59+
install_ci: $(BIN)/thrift install_glide install
7560
ifdef CROSSDOCK
7661
$(MAKE) install_docker_ci
7762
endif
@@ -122,34 +107,23 @@ cover: cover_profile
122107
go tool cover -html=$(BUILD)/coverage.out
123108

124109
cover_ci:
125-
ifdef CROSSDOCK
126-
@echo Skipping coverage
127-
else
110+
@echo "Uploading coverage"
128111
$(MAKE) cover_profile
129-
goveralls -coverprofile=$(BUILD)/coverage.out -service=travis-ci || echo -e "\x1b[31mCoveralls failed\x1b[m"
130-
endif
112+
curl -s https://codecov.io/bash > $(BUILD)/codecov.bash
113+
bash $(BUILD)/codecov.bash -f $(BUILD)/coverage.out
131114

132115

133116
FILTER := grep -v -e '_string.go' -e '/gen-go/' -e '/mocks/' -e 'vendor/'
134117
lint:
135-
ifdef SHOULD_LINT
136-
@echo "Linters are enabled on" $(GO_VERSION)
137118
@echo "Running golint"
138-
-golint ./... | $(FILTER) | tee lint.log
119+
-golint $(ALL_PKGS) | $(FILTER) | tee lint.log
139120
@echo "Running go vet"
140-
-go vet $(PKGS) 2>&1 | tee -a lint.log
141-
ifdef SHOULD_LINT_FMT
142-
@echo "Checking gofmt"
121+
-go vet $(PKGS) 2>&1 | fgrep -v -e "possible formatting directiv" -e "exit status" | tee -a lint.log
122+
@echo "Verifying files are gofmt'd"
143123
-gofmt -l . | $(FILTER) | tee -a lint.log
144-
else
145-
@echo "Not checking gofmt on" $(GO_VERSION)
146-
endif
147124
@echo "Checking for unresolved FIXMEs"
148125
-git grep -i -n fixme | $(FILTER) | grep -v -e Makefile | tee -a lint.log
149126
@[ ! -s lint.log ]
150-
else
151-
@echo "Skipping linters on" $(GO_VERSION)
152-
endif
153127

154128
thrift_example: thrift_gen
155129
go build -o $(BUILD)/examples/thrift ./examples/thrift/main.go

channel.go

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,10 @@ type ChannelOptions struct {
8484
// Note: This is not a stable part of the API and may change.
8585
TimeNow func() time.Time
8686

87+
// TimeTicker is a variable for overriding time.Ticker in unit tests.
88+
// Note: This is not a stable part of the API and may change.
89+
TimeTicker func(d time.Duration) *time.Ticker
90+
8791
// Tracer is an OpenTracing Tracer used to manage distributed tracing spans.
8892
// If not set, opentracing.GlobalTracer() is used.
8993
Tracer opentracing.Tracer
@@ -154,6 +158,7 @@ type channelConnectionCommon struct {
154158
tracer opentracing.Tracer
155159
subChannels *subChannelMap
156160
timeNow func() time.Time
161+
timeTicker func(time.Duration) *time.Ticker
157162
}
158163

159164
// _nextChID is used to allocate unique IDs to every channel for debugging purposes.
@@ -190,10 +195,6 @@ func NewChannel(serviceName string, opts *ChannelOptions) (*Channel, error) {
190195
if logger == nil {
191196
logger = NullLogger
192197
}
193-
logger = logger.WithFields(
194-
LogField{"service", serviceName},
195-
LogField{"process", processName},
196-
)
197198

198199
statsReporter := opts.StatsReporter
199200
if statsReporter == nil {
@@ -205,17 +206,26 @@ func NewChannel(serviceName string, opts *ChannelOptions) (*Channel, error) {
205206
timeNow = time.Now
206207
}
207208

209+
timeTicker := opts.TimeTicker
210+
if timeTicker == nil {
211+
timeTicker = time.NewTicker
212+
}
213+
208214
chID := _nextChID.Inc()
215+
logger = logger.WithFields(
216+
LogField{"serviceName", serviceName},
217+
LogField{"process", processName},
218+
LogField{"chID", chID},
219+
)
220+
209221
ch := &Channel{
210222
channelConnectionCommon: channelConnectionCommon{
211-
log: logger.WithFields(
212-
LogField{"chID", chID},
213-
LogField{"service", serviceName},
214-
LogField{"process", processName}),
223+
log: logger,
215224
relayLocal: toStringSet(opts.RelayLocalHandlers),
216225
statsReporter: statsReporter,
217226
subChannels: &subChannelMap{},
218227
timeNow: timeNow,
228+
timeTicker: timeTicker,
219229
tracer: opts.Tracer,
220230
},
221231
chID: chID,

channel_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,11 +73,11 @@ func TestLoggers(t *testing.T) {
7373

7474
peerInfo := ch.PeerInfo()
7575
fields := toMap(ch.Logger().Fields())
76-
assert.Equal(t, peerInfo.ServiceName, fields["service"])
76+
assert.Equal(t, peerInfo.ServiceName, fields["serviceName"])
7777

7878
sc := ch.GetSubChannel("subch")
7979
fields = toMap(sc.Logger().Fields())
80-
assert.Equal(t, peerInfo.ServiceName, fields["service"])
80+
assert.Equal(t, peerInfo.ServiceName, fields["serviceName"])
8181
assert.Equal(t, "subch", fields["subchannel"])
8282
}
8383

codecov.yml

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
coverage:
2+
range: 75..100
3+
round: down
4+
precision: 2
5+
6+
status:
7+
project:
8+
default:
9+
enabled: yes
10+
target: 85%
11+
if_not_found: success
12+
if_ci_failed: error
13+
ignore:
14+
- "*_string.go"
15+

conn_leak_test.go

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
// Copyright (c) 2017 Uber Technologies, Inc.
2+
3+
// Permission is hereby granted, free of charge, to any person obtaining a copy
4+
// of this software and associated documentation files (the "Software"), to deal
5+
// in the Software without restriction, including without limitation the rights
6+
// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
7+
// copies of the Software, and to permit persons to whom the Software is
8+
// furnished to do so, subject to the following conditions:
9+
//
10+
// The above copyright notice and this permission notice shall be included in
11+
// all copies or substantial portions of the Software.
12+
//
13+
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
14+
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
15+
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
16+
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
17+
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
18+
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
19+
// THE SOFTWARE.
20+
package tchannel_test
21+
22+
import (
23+
"io/ioutil"
24+
"runtime"
25+
"testing"
26+
"time"
27+
28+
. "github.com/uber/tchannel-go"
29+
"github.com/uber/tchannel-go/testutils"
30+
31+
"github.com/stretchr/testify/require"
32+
)
33+
34+
// This is a regression test for https://github.com/uber/tchannel-go/issues/643
35+
// We want to ensure that once a connection is closed, there are no references
36+
// to the closed connection, and the GC frees the connection.
37+
// We use `runtime.SetFinalizer` to detect whether the GC has freed the object.
38+
// However, finalizers cannot be set on objects with circular references,
39+
// so we cannot set a finalizer on the connection, but instead set a finalizer
40+
// on a field of the connection which has the same lifetime. The connection
41+
// logger is unique per connection and does not have circular references
42+
// so we can use the logger, but need a pointer for `runtime.SetFinalizer`.
43+
// loggerPtr is a Logger implementation that uses a pointer unlike other
44+
// TChannel loggers.
45+
type loggerPtr struct {
46+
Logger
47+
}
48+
49+
func (l *loggerPtr) WithFields(fields ...LogField) Logger {
50+
return &loggerPtr{l.Logger.WithFields(fields...)}
51+
}
52+
53+
func TestPeerConnectionLeaks(t *testing.T) {
54+
// Disable log verification since we want to set our own logger.
55+
opts := testutils.NewOpts().NoRelay().DisableLogVerification()
56+
opts.Logger = &loggerPtr{NullLogger}
57+
58+
connFinalized := make(chan struct{})
59+
setFinalizer := func(p *Peer, hostPort string) {
60+
ctx, cancel := NewContext(time.Second)
61+
defer cancel()
62+
63+
conn, err := p.GetConnection(ctx)
64+
require.NoError(t, err, "Failed to get connection")
65+
66+
runtime.SetFinalizer(conn.Logger(), func(interface{}) {
67+
close(connFinalized)
68+
})
69+
}
70+
71+
testutils.WithTestServer(t, opts, func(ts *testutils.TestServer) {
72+
s2Opts := testutils.NewOpts().SetServiceName("s2")
73+
s2Opts.Logger = NewLogger(ioutil.Discard)
74+
s2 := ts.NewServer(s2Opts)
75+
76+
// Set a finalizer to detect when the connection from s1 -> s2 is freed.
77+
peer := ts.Server().Peers().GetOrAdd(s2.PeerInfo().HostPort)
78+
setFinalizer(peer, s2.PeerInfo().HostPort)
79+
80+
// Close s2, so that the connection in s1 to s2 is released.
81+
s2.Close()
82+
closed := testutils.WaitFor(time.Second, s2.Closed)
83+
require.True(t, closed, "s2 didn't close")
84+
85+
// Trigger the GC which will call the finalizer, and ensure
86+
// that the connection logger was finalized.
87+
finalized := testutils.WaitFor(time.Second, func() bool {
88+
runtime.GC()
89+
select {
90+
case <-connFinalized:
91+
return true
92+
default:
93+
return false
94+
}
95+
})
96+
require.True(t, finalized, "Connection was not freed")
97+
})
98+
}

0 commit comments

Comments
 (0)