Skip to content

Commit d42bab5

Browse files
nhachichanhachicha
committed
PR - feedback
1 parent 819810e commit d42bab5

5 files changed

Lines changed: 116 additions & 81 deletions

File tree

‎driver-core/src/main/com/mongodb/internal/connection/BackpressureErrorLabeler.java‎

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -45,21 +45,21 @@ static void applyLabelsIfEligible(final Throwable t) {
4545
if (!(t instanceof MongoSocketException)) {
4646
return;
4747
}
48-
if (isDnsLookupFailure(t)) {
48+
MongoSocketException socketException = (MongoSocketException) t;
49+
if (isDnsLookupFailure(socketException)) {
4950
return;
5051
}
51-
if (isTlsConfigurationError(t)) {
52+
if (isTlsConfigurationError(socketException)) {
5253
return;
5354
}
5455
// TODO-BACKPRESSURE Nabil - SOCKS5 Revisit alongside JAVA-5205 (SOCKS5 in async) so both sync and
5556
// async proxy error surfaces can be handled together — likely via a dedicated internal
5657
// exception thrown from the proxy code path.
57-
MongoException mongoException = (MongoException) t;
58-
mongoException.addLabel(MongoException.SYSTEM_OVERLOADED_ERROR_LABEL);
59-
mongoException.addLabel(MongoException.RETRYABLE_ERROR_LABEL);
58+
socketException.addLabel(MongoException.SYSTEM_OVERLOADED_ERROR_LABEL);
59+
socketException.addLabel(MongoException.RETRYABLE_ERROR_LABEL);
6060
}
6161

62-
private static boolean isDnsLookupFailure(final Throwable t) {
62+
private static boolean isDnsLookupFailure(final MongoSocketException t) {
6363
Throwable cause = t.getCause();
6464
while (cause != null) {
6565
if (cause instanceof UnknownHostException) {
@@ -70,7 +70,7 @@ private static boolean isDnsLookupFailure(final Throwable t) {
7070
return false;
7171
}
7272

73-
private static boolean isTlsConfigurationError(final MongoSocketException t) {
73+
static boolean isTlsConfigurationError(final MongoSocketException t) {
7474
Throwable cause = t.getCause();
7575
while (cause != null) {
7676
if (cause instanceof CertificateException
@@ -90,7 +90,14 @@ private static boolean isTlsConfigurationError(final MongoSocketException t) {
9090
|| lowerMessage.contains("hostname")
9191
|| lowerMessage.contains("protocol")
9292
|| lowerMessage.contains("cipher")
93-
|| lowerMessage.contains("handshake_failure")) {
93+
|| lowerMessage.contains("handshake_failure")
94+
// PKIX path building/validation failures surface as SSLHandshakeException
95+
// when the underlying CertPath* cause is not in the chain.
96+
|| lowerMessage.contains("pkix")
97+
// Any "Received fatal alert: X" from OpenJDK's JSSE provider means the
98+
// server actively answered with a TLS protocol error — not an overload
99+
// signal. Catches all 25 RFC handshake alert descriptions in one rule.
100+
|| lowerMessage.contains("received fatal alert")) {
94101
return true;
95102
}
96103
}

‎driver-core/src/main/com/mongodb/internal/connection/SdamServerDescriptionManager.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -163,7 +163,7 @@ boolean relatedToWriteConcern() {
163163
return exception instanceof MongoWriteConcernWithResponseException;
164164
}
165165

166-
boolean hasBackpressureLabel() {
166+
boolean hasSystemOverloadedLabel() {
167167
return exception instanceof MongoException
168168
&& ((MongoException) exception).hasErrorLabel(MongoException.SYSTEM_OVERLOADED_ERROR_LABEL);
169169
}

‎driver-core/src/test/unit/com/mongodb/internal/connection/BackpressureErrorLabelerTest.java‎

Lines changed: 98 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -23,14 +23,21 @@
2323
import com.mongodb.MongoSocketOpenException;
2424
import com.mongodb.MongoSocketReadTimeoutException;
2525
import com.mongodb.ServerAddress;
26-
import org.bson.BsonDocument;
27-
import org.junit.jupiter.api.Test;
26+
import org.junit.jupiter.api.Named;
27+
import org.junit.jupiter.params.ParameterizedTest;
28+
import org.junit.jupiter.params.provider.MethodSource;
29+
import org.junit.jupiter.params.provider.ValueSource;
2830

2931
import javax.net.ssl.SSLHandshakeException;
3032
import javax.net.ssl.SSLPeerUnverifiedException;
33+
import javax.net.ssl.SSLProtocolException;
34+
import java.io.EOFException;
3135
import java.io.IOException;
3236
import java.net.UnknownHostException;
37+
import java.security.cert.CertPathBuilderException;
38+
import java.security.cert.CertPathValidatorException;
3339
import java.security.cert.CertificateException;
40+
import java.util.stream.Stream;
3441

3542
import static org.junit.jupiter.api.Assertions.assertFalse;
3643
import static org.junit.jupiter.api.Assertions.assertTrue;
@@ -39,105 +46,125 @@ class BackpressureErrorLabelerTest {
3946

4047
private static final ServerAddress ADDRESS = new ServerAddress();
4148

42-
@Test
43-
void mongoSocketExceptionIsLabeled() {
44-
MongoSocketException e = new MongoSocketException("boom", ADDRESS);
45-
BackpressureErrorLabeler.applyLabelsIfEligible(e);
46-
assertHasBackpressureLabels(e);
49+
static Stream<Named<MongoSocketException>> networkErrorShouldBeLabeled() {
50+
return Stream.of(
51+
named(new MongoSocketException("boom", ADDRESS)),
52+
named(new MongoSocketReadTimeoutException("slow", ADDRESS, new IOException("read timed out"))),
53+
named(new MongoSocketOpenException("open failed", ADDRESS, new IOException("connection refused"))),
54+
// FIN-during-handshake: server closed the TCP connection while the client was mid-handshake
55+
// (no protocol-level alert). I/O failure → must be labeled per CMAP "I/O error during TLS handshake".
56+
named(new MongoSocketException("tls", ADDRESS, initCause(
57+
new SSLHandshakeException("Remote host terminated the handshake"),
58+
new EOFException("SSL peer shut down incorrectly"))))
59+
);
4760
}
4861

49-
@Test
50-
void mongoSocketReadTimeoutIsLabeled() {
51-
MongoSocketReadTimeoutException e = new MongoSocketReadTimeoutException("slow", ADDRESS, new IOException("read timed out"));
62+
@ParameterizedTest
63+
@MethodSource
64+
void networkErrorShouldBeLabeled(final MongoSocketException e) {
5265
BackpressureErrorLabeler.applyLabelsIfEligible(e);
5366
assertHasBackpressureLabels(e);
5467
}
5568

56-
@Test
57-
void mongoSocketOpenExceptionIsLabeled() {
58-
MongoSocketOpenException e = new MongoSocketOpenException("open failed", ADDRESS, new IOException("connection refused"));
59-
BackpressureErrorLabeler.applyLabelsIfEligible(e);
60-
assertHasBackpressureLabels(e);
69+
static Stream<Named<MongoSocketException>> dnsFailureShouldNotBeLabeled() {
70+
return Stream.of(
71+
named(new MongoSocketException("lookup failed", ADDRESS, new UnknownHostException("nope"))),
72+
named(new MongoSocketException("wrap", ADDRESS, new IOException("wrap", new UnknownHostException("nope"))))
73+
);
6174
}
6275

63-
@Test
64-
void dnsFailureIsNotLabeled() {
65-
MongoSocketException e = new MongoSocketException("lookup failed", ADDRESS, new UnknownHostException("nope"));
76+
@ParameterizedTest
77+
@MethodSource
78+
void dnsFailureShouldNotBeLabeled(final MongoSocketException e) {
6679
BackpressureErrorLabeler.applyLabelsIfEligible(e);
6780
assertLacksBackpressureLabels(e);
6881
}
6982

70-
@Test
71-
void dnsFailureDeepInCauseChainIsNotLabeled() {
72-
Throwable dns = new UnknownHostException("nope");
73-
IOException wrap1 = new IOException("wrap1", dns);
74-
MongoSocketException e = new MongoSocketException("wrap2", ADDRESS, wrap1);
75-
BackpressureErrorLabeler.applyLabelsIfEligible(e);
76-
assertLacksBackpressureLabels(e);
83+
static Stream<Named<Throwable>> localTlsConfigErrorShouldNotBeLabeled() {
84+
return Stream.of(
85+
named(new CertificateException("bad cert")),
86+
named(new CertPathBuilderException("path build failed")),
87+
named(new CertPathValidatorException("validation failed")),
88+
named(new SSLPeerUnverifiedException("peer not verified")),
89+
named(new SSLProtocolException("protocol error")),
90+
named(new SSLHandshakeException("PKIX path building failed: sun.security.provider.certpath.SunCertPathBuilderException: "
91+
+ "unable to find valid certification path to requested target"))
92+
);
7793
}
7894

79-
@Test
80-
void tlsCertificateErrorIsNotLabeled() {
81-
CertificateException cert = new CertificateException("bad cert");
82-
MongoSocketException e = new MongoSocketException("tls", ADDRESS, cert);
83-
BackpressureErrorLabeler.applyLabelsIfEligible(e);
84-
assertLacksBackpressureLabels(e);
85-
}
86-
87-
@Test
88-
void tlsPeerUnverifiedIsNotLabeled() {
89-
SSLPeerUnverifiedException tls = new SSLPeerUnverifiedException("peer not verified");
90-
MongoSocketException e = new MongoSocketException("tls", ADDRESS, tls);
95+
@ParameterizedTest
96+
@MethodSource
97+
void localTlsConfigErrorShouldNotBeLabeled(final Throwable cause) {
98+
MongoSocketException e = new MongoSocketException("tls", ADDRESS, cause);
9199
BackpressureErrorLabeler.applyLabelsIfEligible(e);
92100
assertLacksBackpressureLabels(e);
93101
}
94102

95-
@Test
96-
void sslHandshakeWithCertificateMessageIsNotLabeled() {
97-
SSLHandshakeException tls = new SSLHandshakeException("PKIX path building failed: certificate chain invalid");
103+
/**
104+
* "Received fatal alert: <description>" means the peer actively answered with a TLS protocol
105+
* error — definitively a config/protocol issue, not an overload signal. Covers all 25
106+
* handshake-only RFC alert descriptions emitted by OpenJDK's JSSE provider.
107+
*/
108+
@ParameterizedTest(name = "Received fatal alert: {0}")
109+
@ValueSource(strings = {
110+
"handshake_failure",
111+
"no_certificate",
112+
"bad_certificate",
113+
"unsupported_certificate",
114+
"certificate_revoked",
115+
"certificate_expired",
116+
"certificate_unknown",
117+
"illegal_parameter",
118+
"unknown_ca",
119+
"access_denied",
120+
"decode_error",
121+
"decrypt_error",
122+
"export_restriction",
123+
"protocol_version",
124+
"insufficient_security",
125+
"no_renegotiation",
126+
"missing_extension",
127+
"unsupported_extension",
128+
"certificate_unobtainable",
129+
"unrecognized_name",
130+
"bad_certificate_status_response",
131+
"bad_certificate_hash_value",
132+
"unknown_psk_identity",
133+
"certificate_required",
134+
"no_application_protocol"
135+
})
136+
void receivedTlsAlertShouldNotBeLabeled(final String alertDescription) {
137+
SSLHandshakeException tls = new SSLHandshakeException("Received fatal alert: " + alertDescription);
98138
MongoSocketException e = new MongoSocketException("tls", ADDRESS, tls);
99139
BackpressureErrorLabeler.applyLabelsIfEligible(e);
100140
assertLacksBackpressureLabels(e);
101141
}
102142

103-
@Test
104-
void sslHandshakeWithoutConfigKeywordIsLabeled() {
105-
SSLHandshakeException tls = new SSLHandshakeException("remote host closed connection during handshake");
106-
MongoSocketException e = new MongoSocketException("tls", ADDRESS, tls);
107-
BackpressureErrorLabeler.applyLabelsIfEligible(e);
108-
assertHasBackpressureLabels(e);
143+
static Stream<Named<Throwable>> nonSocketErrorShouldNotBeLabeled() {
144+
return Stream.of(
145+
named(new MongoSecurityException(
146+
MongoCredential.createCredential("user", "db", "pwd".toCharArray()), "auth failed")),
147+
named(new MongoException(42, "some command error")),
148+
named(new IOException("raw"))
149+
);
109150
}
110151

111-
@Test
112-
void authErrorIsNotLabeled() {
113-
MongoCredential credential = MongoCredential.createCredential("user", "db", "pwd".toCharArray());
114-
MongoSecurityException e = new MongoSecurityException(credential, "auth failed");
152+
@ParameterizedTest
153+
@MethodSource
154+
void nonSocketErrorShouldNotBeLabeled(final Throwable e) {
115155
BackpressureErrorLabeler.applyLabelsIfEligible(e);
116-
assertLacksBackpressureLabels(e);
156+
if (e instanceof MongoException) {
157+
assertLacksBackpressureLabels((MongoException) e);
158+
}
117159
}
118160

119-
@Test
120-
void commandExceptionIsNotLabeled() {
121-
MongoException e = new MongoException(42, "some command error");
122-
BackpressureErrorLabeler.applyLabelsIfEligible(e);
123-
assertLacksBackpressureLabels(e);
161+
private static <T extends Throwable> Named<T> named(final T e) {
162+
return Named.of(e.getClass().getSimpleName(), e);
124163
}
125164

126-
@Test
127-
void nonMongoThrowableIsNotLabeled() {
128-
IOException e = new IOException("raw");
129-
BackpressureErrorLabeler.applyLabelsIfEligible(e);
130-
}
131-
132-
@Test
133-
void sdamIssueTlsCheckDelegatesToHelper() {
134-
MongoSocketException tlsException = new MongoSocketException("tls", ADDRESS, new CertificateException("bad cert"));
135-
assertTrue(BackpressureErrorLabeler.isTlsConfigurationError(tlsException));
136-
137-
MongoSocketException plainException = new MongoSocketException("plain", ADDRESS);
138-
assertFalse(BackpressureErrorLabeler.isTlsConfigurationError(plainException));
139-
140-
assertFalse(BackpressureErrorLabeler.isTlsConfigurationError(new MongoException(0, "not socket", new BsonDocument())));
165+
private static <T extends Throwable> T initCause(final T exception, final Throwable cause) {
166+
exception.initCause(cause);
167+
return exception;
141168
}
142169

143170
private static void assertHasBackpressureLabels(final MongoException e) {

‎driver-core/src/test/unit/com/mongodb/internal/connection/DefaultServerSpecification.groovy‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -263,7 +263,6 @@ class DefaultServerSpecification extends Specification {
263263
def exceptionToThrow = new MongoSocketException('DNS lookup failed', new ServerAddress(),
264264
new UnknownHostException('no such host'))
265265
BackpressureErrorLabeler.applyLabelsIfEligible(exceptionToThrow)
266-
assert !exceptionToThrow.hasErrorLabel(MongoException.SYSTEM_OVERLOADED_ERROR_LABEL)
267266

268267
def connectionPool = Mock(ConnectionPool)
269268
connectionPool.get(_) >> { throw exceptionToThrow }
@@ -276,6 +275,7 @@ class DefaultServerSpecification extends Specification {
276275
then:
277276
def e = thrown(MongoException)
278277
e.is(exceptionToThrow)
278+
!e.hasErrorLabel(MongoException.SYSTEM_OVERLOADED_ERROR_LABEL)
279279
1 * connectionPool.invalidate(exceptionToThrow)
280280
1 * serverMonitor.cancelCurrentCheck()
281281
}

‎driver-sync/src/test/functional/com/mongodb/client/unified/UnifiedTestModifications.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -439,6 +439,7 @@ public static void applyCustomizations(final TestDef def) {
439439
.file("server-discovery-and-monitoring", "pool-clear-on-error-checkout");
440440
def.skipJira("https://jira.mongodb.org/browse/JAVA-5664")
441441
.file("server-discovery-and-monitoring", "pool-cleared-on-min-pool-size-population-error");
442+
// TODO-BACKPRESSURE Nabil - This issue is unrelated to backpressure but consider fixing it before merging to main
442443
def.skipJira("https://jira.mongodb.org/browse/JAVA-6174")
443444
.file("server-discovery-and-monitoring", "backpressure-server-description-unchanged-on-min-pool-size-population-error");
444445

0 commit comments

Comments
 (0)