-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Accept long
instead of int
in SocketSettings.Builder.connectTimeout
/readTimeout
#1279
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,9 +20,11 @@ | |
import com.mongodb.ConnectionString; | ||
import com.mongodb.annotations.Immutable; | ||
|
||
import java.util.Objects; | ||
import java.util.concurrent.TimeUnit; | ||
|
||
import static com.mongodb.assertions.Assertions.notNull; | ||
import static java.lang.Math.toIntExact; | ||
import static java.util.concurrent.TimeUnit.MILLISECONDS; | ||
|
||
/** | ||
|
@@ -32,8 +34,8 @@ | |
*/ | ||
@Immutable | ||
public final class SocketSettings { | ||
private final long connectTimeoutMS; | ||
private final long readTimeoutMS; | ||
private final int connectTimeoutMS; | ||
private final int readTimeoutMS; | ||
private final int receiveBufferSize; | ||
private final int sendBufferSize; | ||
private final ProxySettings proxySettings; | ||
|
@@ -62,8 +64,8 @@ public static Builder builder(final SocketSettings socketSettings) { | |
* A builder for an instance of {@code SocketSettings}. | ||
*/ | ||
public static final class Builder { | ||
private long connectTimeoutMS = 10000; | ||
private long readTimeoutMS; | ||
private int connectTimeoutMS = 10000; | ||
private int readTimeoutMS; | ||
private int receiveBufferSize; | ||
private int sendBufferSize; | ||
private ProxySettings.Builder proxySettingsBuilder = ProxySettings.builder(); | ||
|
@@ -93,25 +95,27 @@ public Builder applySettings(final SocketSettings socketSettings) { | |
/** | ||
* Sets the socket connect timeout. | ||
* | ||
* @param connectTimeout the connect timeout | ||
* @param connectTimeout the connect timeout. | ||
* The timeout converted to milliseconds must not be greater than {@link Integer#MAX_VALUE}. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We had this requirement for forever (the |
||
* @param timeUnit the time unit | ||
* @return this | ||
*/ | ||
public Builder connectTimeout(final int connectTimeout, final TimeUnit timeUnit) { | ||
this.connectTimeoutMS = MILLISECONDS.convert(connectTimeout, timeUnit); | ||
public Builder connectTimeout(final long connectTimeout, final TimeUnit timeUnit) { | ||
this.connectTimeoutMS = timeoutArgumentToMillis(connectTimeout, timeUnit); | ||
return this; | ||
} | ||
|
||
/** | ||
* Sets the socket read timeout. | ||
* | ||
* @param readTimeout the read timeout | ||
* @param readTimeout the read timeout. | ||
* The timeout converted to milliseconds must not be greater than {@link Integer#MAX_VALUE}. | ||
* @param timeUnit the time unit | ||
* @return this | ||
* @see #getReadTimeout(TimeUnit) | ||
*/ | ||
public Builder readTimeout(final int readTimeout, final TimeUnit timeUnit) { | ||
this.readTimeoutMS = MILLISECONDS.convert(readTimeout, timeUnit); | ||
public Builder readTimeout(final long readTimeout, final TimeUnit timeUnit) { | ||
this.readTimeoutMS = timeoutArgumentToMillis(readTimeout, timeUnit); | ||
return this; | ||
} | ||
|
||
|
@@ -197,7 +201,7 @@ public int getConnectTimeout(final TimeUnit timeUnit) { | |
* | ||
* @param timeUnit the time unit to get the timeout in | ||
* @return the read timeout in the requested time unit, or 0 if there is no timeout | ||
* @see Builder#readTimeout(int, TimeUnit) | ||
* @see Builder#readTimeout(long, TimeUnit) | ||
*/ | ||
public int getReadTimeout(final TimeUnit timeUnit) { | ||
return (int) timeUnit.convert(readTimeoutMS, MILLISECONDS); | ||
|
@@ -260,12 +264,7 @@ public boolean equals(final Object o) { | |
|
||
@Override | ||
public int hashCode() { | ||
int result = (int) (connectTimeoutMS ^ (connectTimeoutMS >>> 32)); | ||
result = 31 * result + (int) (readTimeoutMS ^ (readTimeoutMS >>> 32)); | ||
result = 31 * result + receiveBufferSize; | ||
result = 31 * result + sendBufferSize; | ||
result = 31 * result + proxySettings.hashCode(); | ||
return result; | ||
Comment on lines
-263
to
-268
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The bit shifting in the old code became irrelevant, i.e., |
||
return Objects.hash(connectTimeoutMS, readTimeoutMS, receiveBufferSize, sendBufferSize, proxySettings); | ||
} | ||
|
||
@Override | ||
|
@@ -285,4 +284,13 @@ private SocketSettings(final Builder builder) { | |
sendBufferSize = builder.sendBufferSize; | ||
proxySettings = builder.proxySettingsBuilder.build(); | ||
} | ||
|
||
private static int timeoutArgumentToMillis(final long timeout, final TimeUnit timeUnit) throws IllegalArgumentException { | ||
try { | ||
return toIntExact(MILLISECONDS.convert(timeout, timeUnit)); | ||
} catch (ArithmeticException e) { | ||
throw new IllegalArgumentException( | ||
"The timeout converted to milliseconds must not be greater than `Integer.MAX_VALUE`", e); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,7 +21,9 @@ import spock.lang.Specification | |
|
||
import static java.util.concurrent.TimeUnit.MILLISECONDS | ||
|
||
|
||
/** | ||
* New unit tests for {@link SocketSettings} are to be added to {@link SocketSettingsTest}. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The team discussed and made a decision to create new tests in Java where this is not too much of a hassle, even if there are existing tests in Groovy. |
||
*/ | ||
class SocketSettingsSpecification extends Specification { | ||
|
||
def 'should have correct defaults'() { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
/* | ||
* Copyright 2008-present MongoDB, Inc. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
package com.mongodb.connection; | ||
|
||
import org.junit.jupiter.api.Test; | ||
|
||
import java.util.concurrent.TimeUnit; | ||
|
||
import static org.junit.jupiter.api.Assertions.assertThrows; | ||
|
||
/** | ||
* {@link SocketSettingsSpecification} contains older unit tests for {@link SocketSettings}. | ||
*/ | ||
final class SocketSettingsTest { | ||
@Test | ||
void connectTimeoutThrowsIfArgumentIsTooLarge() { | ||
assertThrows(IllegalArgumentException.class, () -> SocketSettings.builder().connectTimeout(Integer.MAX_VALUE / 2, TimeUnit.SECONDS)); | ||
} | ||
|
||
@Test | ||
void readTimeoutThrowsIfArgumentIsTooLarge() { | ||
assertThrows(IllegalArgumentException.class, () -> SocketSettings.builder().readTimeout(Integer.MAX_VALUE / 2, TimeUnit.SECONDS)); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no reason to have the fields internally of the
long
type, because the millisecond values were not allowed (and will continue being not allowed) to be greater thanInteger.MAX_VALUE
.