Skip to content

Commit 07079a7

Browse files
committed
- FlowControl: send back credits for a credit request for which we don't have a member (2nd line of defense) - GMS.installView(): moved sending down and up of VIEW_CHANGE back into the lock scope (https://redhat.atlassian.net/browse/JGRP-3036) - GMS.installView(): moved sending down and up of VIEW_CHANGE back into the lock scope (https://redhat.atlassian.net/browse/JGRP-3036) - Repro1048: reproducer by ocherechin, changed into a unit test - Util: added failSafe()
1 parent a0b5fc9 commit 07079a7

5 files changed

Lines changed: 246 additions & 13 deletions

File tree

src/org/jgroups/logging/Log.java

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,4 +54,49 @@ public interface Log {
5454
void setLevel(String level);
5555

5656
String getLevel();
57+
58+
default void failSafeWarn(String msg) {
59+
try {
60+
warn(msg);
61+
}
62+
catch(Throwable t) {
63+
}
64+
}
65+
default void failSafeWarn(String format, Object ... args) {
66+
try {
67+
warn(format, args);
68+
}
69+
catch(Throwable t) {
70+
}
71+
}
72+
73+
default void failSafeWarn(String msg, Throwable throwable) {
74+
try {
75+
warn(msg, throwable);
76+
}
77+
catch(Throwable t) {
78+
}
79+
}
80+
81+
default void failSafeError(String msg) {
82+
try {
83+
error(msg);
84+
}
85+
catch(Throwable t) {}
86+
}
87+
88+
default void failSafeError(String format, Object ... args) {
89+
try {
90+
error(format, args);
91+
}
92+
catch(Throwable t) {
93+
}
94+
}
95+
96+
default void failSafeError(String msg, Throwable throwable) {
97+
try {
98+
error(msg, throwable);
99+
}
100+
catch(Throwable t) {}
101+
}
57102
}

src/org/jgroups/protocols/FlowControl.java

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -367,9 +367,7 @@ protected long adjustCredit(Map<Address,Credit> map, Address sender, int length)
367367
*/
368368
protected void handleCreditRequest(Map<Address,Credit> map, Address sender, long requested_credits) {
369369
if(requested_credits > 0 && sender != null) {
370-
Credit cred=map.get(sender);
371-
if(cred == null)
372-
return;
370+
Credit cred=map.putIfAbsent(sender, new Credit(max_credits)); // https://redhat.atlassian.net/browse/JGRP-3036
373371
if(log.isTraceEnabled())
374372
log.trace("received credit request from %s: sending %d credits", sender, requested_credits);
375373
cred.increment(requested_credits, max_credits);
@@ -402,7 +400,6 @@ protected void sendCreditRequest(final Address dest, long credits_needed) {
402400
num_credit_requests_sent++;
403401
}
404402

405-
406403
protected void handleViewChange(List<Address> mbrs) {
407404
if(mbrs == null) return;
408405
if(log.isTraceEnabled()) log.trace("new membership: %s", mbrs);
@@ -414,8 +411,6 @@ protected void handleViewChange(List<Address> mbrs) {
414411
received.keySet().retainAll(mbrs);
415412
}
416413

417-
418-
419414
protected static String printMap(Map<Address,? extends Credit> m) {
420415
return m.entrySet().stream().map(e -> String.format("%s: %s", e.getKey(), e.getValue()))
421416
.collect(Collectors.joining("\n"));

src/org/jgroups/protocols/pbcast/GMS.java

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -681,17 +681,17 @@ public void installView(View new_view, Digest digest) {
681681
prev_views.add(Util.utcNow() + ": " + new_view);
682682
}
683683
am_i_coord=Objects.equals(local_addr, new_view.getCoord());
684+
685+
// - Changed order of passing view up and down (https://issues.redhat.com/browse/JGRP-347)
686+
// - Changed it back (bela Sept 4 2007): https://issues.redhat.com/browse/JGRP-564
687+
// - Moved sending up view_event out of the synchronized block (bela Nov 2011)
688+
// - Moved back inside the lock scope: https://redhat.atlassian.net/browse/JGRP-3036 (bela Aug 2026)
689+
Util.failSafe(() -> down_prot.down(view_event), this.log); // needed e.g. by failure detector or UDP
690+
Util.failSafe(() -> up_prot.up(view_event), this.log);
684691
}
685692
finally {
686693
lock.unlock();
687694
}
688-
689-
// - Changed order of passing view up and down (https://issues.redhat.com/browse/JGRP-347)
690-
// - Changed it back (bela Sept 4 2007): https://issues.redhat.com/browse/JGRP-564
691-
// - Moved sending up view_event out of the synchronized block (bela Nov 2011)
692-
down_prot.down(view_event); // needed e.g. by failure detector or UDP
693-
up_prot.up(view_event);
694-
695695
// Everybody except the merge leader cancels the merge, otherwise - if UNICAST3.loopback is true - we'd
696696
// interrupt our own thread which will fail code that later sends a message before returning!
697697
// Note that the merge leader does cancel the merge later, after having installed the MergeView

src/org/jgroups/util/Util.java

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -442,6 +442,19 @@ public static <T> void waitUntilListHasSize(List<T> list,int expected_size,long
442442
assert list.size() == expected_size : "list doesn't have the expected (" + expected_size + ") elements: " + list;
443443
}
444444

445+
/** Executes a runnable without throwing an exception */
446+
public static void failSafe(Runnable r, Log log) {
447+
if(r != null) {
448+
try {
449+
r.run();
450+
}
451+
catch(Throwable t) {
452+
if(log != null)
453+
log.failSafeError("failed execution task %s: %s", r, t);
454+
}
455+
}
456+
}
457+
445458
public static void removeFromViews(Address mbr, JChannel ... channels) {
446459
if(mbr == null || channels == null)
447460
return;
Lines changed: 180 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,180 @@
1+
package org.jgroups.tests;
2+
3+
import org.jgroups.*;
4+
import org.jgroups.protocols.FRAG4;
5+
import org.jgroups.protocols.TP;
6+
import org.jgroups.protocols.UFC;
7+
import org.jgroups.protocols.pbcast.GMS;
8+
import org.jgroups.stack.Protocol;
9+
import org.jgroups.stack.ProtocolStack;
10+
import org.jgroups.util.Util;
11+
import org.testng.annotations.Test;
12+
13+
import java.util.ArrayList;
14+
import java.util.List;
15+
import java.util.stream.Collectors;
16+
import java.util.stream.Stream;
17+
18+
/**
19+
* Single-JVM reproducer for JGroups issue 1048: a stale view reaches FlowControl after a newer one,
20+
* and evicts a member that is still in the cluster.
21+
*
22+
* WHAT IT SHOWS
23+
*
24+
* GMS.installView() guards against stale views correctly, but it sets `view` under the lock and
25+
* dispatches after releasing it (5.5.1 lines 691-699):
26+
*
27+
* finally { lock.unlock(); }
28+
* down_prot.down(view_event);
29+
* up_prot.up(view_event);
30+
*
31+
* So installation order and dispatch order are separate. Two threads can be inside installView()
32+
* for different views, both passing the guard legitimately: the joining application thread inside
33+
* connect() (ClientGmsImpl.installViewIfValidJoinRsp -> GMS.installView) and a JGroups thread
34+
* handling the coordinator's next view. If anything below GMS is slow for the older view's
35+
* down_prot.down(), the newer view completes its whole dispatch first and the older one arrives at
36+
* FlowControl afterwards. FlowControl.up() has no view-id check, so handleViewChange() runs with
37+
* the older member list and retainAll() drops a live member. handleCreditRequest() then does
38+
* map.get(sender), finds null, and returns without replenishing: that peer's credit window drains
39+
* one way and never refills.
40+
*
41+
* The only thing simulated here is "something below GMS is slow", by SlowViewDown below. In
42+
* production that was FD_SOCK2/TCP setting up connections to a member that was itself still
43+
* starting; measured stalls there were 6.0s (four occurrences) and 8.03s.
44+
*
45+
* JIRA https://redhat.atlassian.net/browse/JGRP-3036
46+
*/
47+
@Test(groups=Global.FUNCTIONAL)
48+
public class Repro1048 {
49+
static final String CLUSTER="repro1048";
50+
51+
public void test1048() throws Exception {
52+
JChannel a=channel("A", false);
53+
JChannel b=channel("B", true);
54+
JChannel c=channel("C", false);
55+
try {
56+
a.connect(CLUSTER);
57+
say("A connected, view=%s", a.getView());
58+
59+
// B joins on an APPLICATION thread, which is what connect() is in a real deployment.
60+
// Its first view's down-dispatch is then held, so connect() does not return yet.
61+
Thread joiner=new Thread(() -> {
62+
try {
63+
b.connect(CLUSTER);
64+
}
65+
catch(Exception e) {
66+
e.printStackTrace();
67+
}
68+
}, "app-joiner-B");
69+
joiner.start();
70+
71+
// Let B install its first view and enter the stall, then let C join. The coordinator moves to a 3-member
72+
// view and B applies it on a JGroups thread, while B's own 2-member view is still undelivered.
73+
Util.sleep(2000);
74+
c.connect(CLUSTER);
75+
say("C connected, view=%s", c.getView());
76+
77+
joiner.join();
78+
say("B connect() returned, view=%s", b.getView());
79+
Util.sleep(1000);
80+
81+
System.out.printf("\n-------------- channel views:\n%s\n",
82+
Stream.of(a,b,c).map(ch -> String.format("%s: %s", ch.address(), ch.view()))
83+
.collect(Collectors.joining("\n")));
84+
85+
System.out.printf("\n-------------- GMS views:\n%s\n\n",
86+
Stream.of(a,b,c)
87+
.map(ch -> ch.stack().findProtocol(GMS.class))
88+
.map(gms -> String.format("%s: %s", ((GMS)gms).addr(), ((GMS)gms).view()))
89+
.collect(Collectors.joining("\n")));
90+
91+
// The coordinator's view is authoritative: every member in it is alive.
92+
View cluster = a.getView();
93+
List<Address> members = cluster.getMembers();
94+
UFC ufc = b.getProtocolStack().findProtocol(UFC.class);
95+
String senders = ufc.printSenderCredits();
96+
String recv = ufc.printReceiverCredits();
97+
98+
List<Address> missing=new ArrayList<>();
99+
for(Address m: members)
100+
if(!inMap(senders, m) || !inMap(recv, m))
101+
missing.add(m);
102+
103+
say("cluster view (from coordinator A): %s", cluster);
104+
say("B's channel view: %s", b.getView());
105+
say("B's UFC credits:%n%s", ufc.printCredits());
106+
107+
if(missing.isEmpty()) {
108+
say("NOT REPRODUCED: B's credit maps contain every member of %s", cluster.getViewId());
109+
return;
110+
}
111+
say("REPRODUCED: %s is in view %s but missing from B's UFC maps."
112+
+ " handleCreditRequest() will drop its credit requests, and its window drains one way.",
113+
missing, cluster.getViewId());
114+
assert false : String.format("REPRODUCED: %s is in view %s but missing from B's UFC maps."
115+
+ " handleCreditRequest() will drop its credit requests, and its window drains one way.",
116+
missing, cluster.getViewId());
117+
}
118+
finally {
119+
Util.close(c, b, a);
120+
}
121+
}
122+
123+
/** Minimal TCP stack. SlowViewDown sits directly above the transport, where a slow protocol is. */
124+
static JChannel channel(String name, boolean slow) throws Exception {
125+
JChannel ch=new JChannel(Util.getTestStack(new UFC().setMaxCredits(2_000_000), new FRAG4())).name(name);
126+
if(slow) {
127+
SlowViewDown sv=new SlowViewDown().delay(4000);
128+
ch.stack().insertProtocol(sv, ProtocolStack.Position.ABOVE, TP.class);
129+
}
130+
return ch;
131+
}
132+
133+
/** printMap() emits "<addr>: <credits>" per line. */
134+
static boolean inMap(String credit_map, Address mbr) {
135+
for(String line: credit_map.split("\n")) {
136+
int idx=line.lastIndexOf(':');
137+
if(idx > 0 && line.substring(0, idx).trim().equals(mbr.toString()))
138+
return true;
139+
}
140+
return false;
141+
}
142+
143+
static void say(String fmt, Object... args) {
144+
//noinspection StringConcatenationInFormatCall
145+
System.out.printf("%tT.%<tL " + fmt + "%n", prepend(System.currentTimeMillis(), args));
146+
}
147+
148+
static Object[] prepend(Object first, Object[] rest) {
149+
Object[] all=new Object[rest.length + 1];
150+
all[0]=first;
151+
System.arraycopy(rest, 0, all, 1, rest.length);
152+
return all;
153+
}
154+
155+
/**
156+
* Holds the down-dispatch of a view, and only when the dispatching thread is not a JGroups
157+
* stack thread. That restriction matters: delaying a stack thread blocks that sender's message
158+
* queue, so the newer view would never arrive and nothing would reproduce.
159+
*/
160+
public static class SlowViewDown extends Protocol {
161+
protected long delay=8000;
162+
163+
public SlowViewDown delay(long d) {delay=d; return this;}
164+
165+
public Object down(Event evt) {
166+
if(evt.getType() == Event.VIEW_CHANGE) {
167+
View v = evt.getArg();
168+
String thread = Thread.currentThread().getName();
169+
boolean off_stack=!thread.startsWith("jgroups-") && !thread.startsWith("thread-");
170+
if(v.getViewId().getId() <= 1 && off_stack) {
171+
say(" [SlowViewDown] holding down-dispatch of %s for %dms on thread %s",
172+
v.getViewId(), delay, thread);
173+
Util.sleep(delay);
174+
say(" [SlowViewDown] releasing %s", v.getViewId());
175+
}
176+
}
177+
return down_prot.down(evt);
178+
}
179+
}
180+
}

0 commit comments

Comments
 (0)