Skip to content

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

Merged
merged 2 commits into from
Jan 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 25 additions & 17 deletions driver-core/src/main/com/mongodb/connection/SocketSettings.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Comment on lines +67 to +68
Copy link
Member Author

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 than Integer.MAX_VALUE.

private int receiveBufferSize;
private int sendBufferSize;
private ProxySettings.Builder proxySettingsBuilder = ProxySettings.builder();
Expand Down Expand Up @@ -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}.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had this requirement for forever (the toIntExact conversion does not, therefore, introduce a new restriction), but did not document it. Now it is documented.

* @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;
}

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bit shifting in the old code became irrelevant, i.e., hashCode had to be updated. I updated it to use Objects.hash.

return Objects.hash(connectTimeoutMS, readTimeoutMS, receiveBufferSize, sendBufferSize, proxySettings);
}

@Override
Expand All @@ -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
Expand Up @@ -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}.
Copy link
Member Author

Choose a reason for hiding this comment

The 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'() {
Expand Down
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));
}
}