Skip to content

Commit 9225cc8

Browse files
authored
fix(watch): do not drop events when a read carries multiple responses (#224)
1 parent bbcaf8a commit 9225cc8

4 files changed

Lines changed: 242 additions & 7 deletions

File tree

.github/workflows/ci.yml

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ jobs:
2222
- version: 3.4.0
2323
conf: Procfile-single-enable-mtls
2424

25-
runs-on: "ubuntu-20.04"
25+
runs-on: "ubuntu-22.04"
2626
env:
2727
OPENRESTY_PREFIX: "/usr/local/openresty"
2828
AUTH_ENDPOINT_V3: "127.0.0.1:12379"
@@ -35,10 +35,12 @@ jobs:
3535
- uses: actions/checkout@v2
3636

3737
- name: setup go
38-
uses: actions/setup-go@v2.1.3
38+
uses: actions/setup-go@v5
3939
with:
40-
# goreman requires 1.17
41-
go-version: "1.17"
40+
# Keep this in sync with the goreman pin in the install step: it must
41+
# be >= goreman's module `go` directive, otherwise the toolchain gets
42+
# downloaded implicitly (see GOTOOLCHAIN=local note below).
43+
go-version: "1.22"
4244

4345
- name: get dependencies
4446
run: sudo apt install -y cpanminus build-essential libncurses5-dev libreadline-dev libssl-dev perl
@@ -68,7 +70,12 @@ jobs:
6870
tar xf etcd-v$ETCD_VER-linux-amd64.tar.gz
6971
# run etcd local cluster, startup at localhost:2379, localhost:22379, and localhost:32379
7072
# see more https://github.com/etcd-io/etcd/blob/master/Documentation/dev-guide/local_cluster.md
71-
go get github.com/mattn/goreman
73+
# Pin goreman to a fixed release for reproducible CI. v0.3.15 declares
74+
# `go 1.19` and pulls golang.org/x/sys v0.6.0, both of which build with
75+
# the pinned toolchain above. GOTOOLCHAIN=local forbids Go from
76+
# silently downloading a newer toolchain (goreman >= v0.3.19 declares
77+
# `go 1.25`, which would otherwise defeat the setup-go pin).
78+
GOTOOLCHAIN=local go install github.com/mattn/goreman@v0.3.15
7279
7380
- name: script
7481
if: matrix.conf != 'Procfile-single-enable-mtls'

lib/resty/etcd/v3.lua

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -939,7 +939,12 @@ local function request_chunk(self, method, path, opts, timeout)
939939
end
940940
end
941941

942-
if #all_events > 1 then
942+
-- one read may carry several watch responses separated by "\n", and the
943+
-- last one is not necessarily the one holding the events, e.g.
944+
-- [response with events, progress notification]. `body` points at the
945+
-- last decoded response, so the collected events must always be
946+
-- attached to it, not only when more than one event was collected.
947+
if #all_events > 0 and body.result then
943948
body.result.events = all_events
944949
end
945950
return body

t/v3/tls.t

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ GET /t
103103
--- no_error_log
104104
[error]
105105
--- response_body eval
106-
qr/18: self signed certificate/
106+
qr/18: self[ -]signed certificate/
107107
108108
109109

t/v3/watch.t

Lines changed: 223 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,223 @@
1+
use Test::Nginx::Socket::Lua;
2+
3+
log_level('info');
4+
no_long_string();
5+
repeat_each(1);
6+
plan 'no_plan';
7+
8+
# etcd coalesces watch responses into a single body read whenever they are
9+
# written close enough together (very common once progress_notify is on), and a
10+
# real etcd cannot be driven into that state reliably -- so each test below uses
11+
# a fake endpoint that emits a fixed, hand-crafted coalesced read. The point of
12+
# the fix under test is that the response holding the events is NOT necessarily
13+
# the last one in the read, so we exercise both orderings and an interleaving.
14+
15+
# [event, progress]: event (rev 7) followed by a progress notification (rev 9).
16+
our $HttpConfig = <<'_EOC_';
17+
lua_socket_log_errors off;
18+
lua_package_path 'lib/?.lua;/usr/local/share/lua/5.3/?.lua;/usr/share/lua/5.1/?.lua;;';
19+
server {
20+
listen 1985;
21+
location /v3/watch {
22+
content_by_lua_block {
23+
ngx.print('{"result":{"header":{"revision":"7"},"events":' ..
24+
'[{"type":"PUT","kv":{"key":"L3Rlc3Q=","value":"ImFiYyI=",' ..
25+
'"mod_revision":"7"}}]}}\n' ..
26+
'{"result":{"header":{"revision":"9"}}}\n')
27+
}
28+
}
29+
}
30+
_EOC_
31+
32+
# [progress, event]: progress notification (rev 7) followed by an event (rev 9).
33+
our $HttpConfigProgressFirst = <<'_EOC_';
34+
lua_socket_log_errors off;
35+
lua_package_path 'lib/?.lua;/usr/local/share/lua/5.3/?.lua;/usr/share/lua/5.1/?.lua;;';
36+
server {
37+
listen 1985;
38+
location /v3/watch {
39+
content_by_lua_block {
40+
ngx.print('{"result":{"header":{"revision":"7"}}}\n' ..
41+
'{"result":{"header":{"revision":"9"},"events":' ..
42+
'[{"type":"PUT","kv":{"key":"L3Rlc3Q=","value":"ImFiYyI=",' ..
43+
'"mod_revision":"9"}}]}}\n')
44+
}
45+
}
46+
}
47+
_EOC_
48+
49+
# [event, progress, event]: an event (rev 7), a progress notification (rev 8),
50+
# and a second event (rev 9) all in one read.
51+
our $HttpConfigInterleaved = <<'_EOC_';
52+
lua_socket_log_errors off;
53+
lua_package_path 'lib/?.lua;/usr/local/share/lua/5.3/?.lua;/usr/share/lua/5.1/?.lua;;';
54+
server {
55+
listen 1985;
56+
location /v3/watch {
57+
content_by_lua_block {
58+
ngx.print('{"result":{"header":{"revision":"7"},"events":' ..
59+
'[{"type":"PUT","kv":{"key":"L3Rlc3Q=","value":"ImFiYyI=",' ..
60+
'"mod_revision":"7"}}]}}\n' ..
61+
'{"result":{"header":{"revision":"8"}}}\n' ..
62+
'{"result":{"header":{"revision":"9"},"events":' ..
63+
'[{"type":"PUT","kv":{"key":"L3Rlc3Qy","value":"ImJjZCI=",' ..
64+
'"mod_revision":"9"}}]}}\n')
65+
}
66+
}
67+
}
68+
_EOC_
69+
70+
run_tests();
71+
72+
__DATA__
73+
74+
=== TEST 1: a coalesced read keeps the events of every response it contains
75+
--- http_config eval: $::HttpConfig
76+
--- config
77+
location /t {
78+
content_by_lua_block {
79+
local etcd, err = require "resty.etcd" .new({
80+
protocol = "v3",
81+
http_host = "http://127.0.0.1:1985",
82+
})
83+
if not etcd then
84+
ngx.say("failed to new: ", err)
85+
return
86+
end
87+
88+
local res_func, err = etcd:watchdir("/test", {timeout = 5})
89+
if not res_func then
90+
ngx.say("failed to watchdir: ", err)
91+
return
92+
end
93+
94+
local res, err = res_func()
95+
if not res then
96+
ngx.say("failed to read watch: ", err)
97+
return
98+
end
99+
100+
-- The read is [event(rev 7), progress(rev 9)]. We report the
101+
-- progress notification's revision (9, the last response in the
102+
-- read) while still delivering the rev-7 event: a progress
103+
-- notification guarantees nothing happened between the event and
104+
-- it, so the caller can safely advance start_revision past 9.
105+
ngx.say("revision: ", res.result.header.revision)
106+
local events = res.result.events
107+
ngx.say("events: ", events and #events or 0)
108+
if events and events[1] then
109+
ngx.say("key: ", events[1].kv.key)
110+
ngx.say("value: ", events[1].kv.value)
111+
end
112+
}
113+
}
114+
--- request
115+
GET /t
116+
--- response_body
117+
revision: 9
118+
events: 1
119+
key: /test
120+
value: abc
121+
--- no_error_log
122+
[error]
123+
124+
125+
126+
=== TEST 2: a coalesced read keeps the event when it is the last response
127+
--- http_config eval: $::HttpConfigProgressFirst
128+
--- config
129+
location /t {
130+
content_by_lua_block {
131+
local etcd, err = require "resty.etcd" .new({
132+
protocol = "v3",
133+
http_host = "http://127.0.0.1:1985",
134+
})
135+
if not etcd then
136+
ngx.say("failed to new: ", err)
137+
return
138+
end
139+
140+
local res_func, err = etcd:watchdir("/test", {timeout = 5})
141+
if not res_func then
142+
ngx.say("failed to watchdir: ", err)
143+
return
144+
end
145+
146+
local res, err = res_func()
147+
if not res then
148+
ngx.say("failed to read watch: ", err)
149+
return
150+
end
151+
152+
-- The read is [progress(rev 7), event(rev 9)]. The event is the
153+
-- last response, so its revision (9) is reported and its event
154+
-- delivered.
155+
ngx.say("revision: ", res.result.header.revision)
156+
local events = res.result.events
157+
ngx.say("events: ", events and #events or 0)
158+
if events and events[1] then
159+
ngx.say("key: ", events[1].kv.key)
160+
ngx.say("value: ", events[1].kv.value)
161+
end
162+
}
163+
}
164+
--- request
165+
GET /t
166+
--- response_body
167+
revision: 9
168+
events: 1
169+
key: /test
170+
value: abc
171+
--- no_error_log
172+
[error]
173+
174+
175+
176+
=== TEST 3: a coalesced read keeps events from every response when interleaved
177+
--- http_config eval: $::HttpConfigInterleaved
178+
--- config
179+
location /t {
180+
content_by_lua_block {
181+
local etcd, err = require "resty.etcd" .new({
182+
protocol = "v3",
183+
http_host = "http://127.0.0.1:1985",
184+
})
185+
if not etcd then
186+
ngx.say("failed to new: ", err)
187+
return
188+
end
189+
190+
local res_func, err = etcd:watchdir("/test", {timeout = 5})
191+
if not res_func then
192+
ngx.say("failed to watchdir: ", err)
193+
return
194+
end
195+
196+
local res, err = res_func()
197+
if not res then
198+
ngx.say("failed to read watch: ", err)
199+
return
200+
end
201+
202+
-- The read is [event(rev 7), progress(rev 8), event(rev 9)]. Both
203+
-- events must survive, in order, under the highest revision (9).
204+
ngx.say("revision: ", res.result.header.revision)
205+
local events = res.result.events
206+
ngx.say("events: ", events and #events or 0)
207+
if events then
208+
for i = 1, #events do
209+
ngx.say("key", i, ": ", events[i].kv.key,
210+
" value", i, ": ", events[i].kv.value)
211+
end
212+
end
213+
}
214+
}
215+
--- request
216+
GET /t
217+
--- response_body
218+
revision: 9
219+
events: 2
220+
key1: /test value1: abc
221+
key2: /test2 value2: bcd
222+
--- no_error_log
223+
[error]

0 commit comments

Comments
 (0)