Skip to content

feat: Make writeTimeout configurable (similar to readTimeout) #900

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 3 commits into from
May 15, 2024
Merged
Show file tree
Hide file tree
Changes from 2 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
28 changes: 28 additions & 0 deletions src/main/java/com/google/firebase/FirebaseOptions.java
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ public GoogleCredentials get() {
private final HttpTransport httpTransport;
private final int connectTimeout;
private final int readTimeout;
private final int writeTimeout;
private final JsonFactory jsonFactory;
private final ThreadManager threadManager;
private final FirestoreOptions firestoreOptions;
Expand Down Expand Up @@ -112,6 +113,8 @@ private FirebaseOptions(@NonNull final FirebaseOptions.Builder builder) {
this.connectTimeout = builder.connectTimeout;
checkArgument(builder.readTimeout >= 0);
this.readTimeout = builder.readTimeout;
checkArgument(builder.writeTimeout >= 0);
this.writeTimeout = builder.writeTimeout;
this.firestoreOptions = builder.firestoreOptions;
}

Expand Down Expand Up @@ -207,6 +210,16 @@ public int getReadTimeout() {
return readTimeout;
}

/**
* Returns the write timeout in milliseconds, which is applied to outgoing REST calls
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is a case for "that" instead of which, let's just rephrase to something like:

"Returns the write timeout applied to outgoing REST calls in milliseconds."

If you think we can go ahead and assume "by the SDK."

* made by the SDK.
*
* @return Write timeout in milliseconds. 0 indicates an infinite timeout.
*/
public int getWriteTimeout() {
return writeTimeout;
}

@NonNull
ThreadManager getThreadManager() {
return threadManager;
Expand Down Expand Up @@ -260,6 +273,7 @@ public static final class Builder {
private ThreadManager threadManager;
private int connectTimeout;
private int readTimeout;
private int writeTimeout;

/**
* Constructs an empty builder.
Expand Down Expand Up @@ -289,6 +303,7 @@ public Builder(FirebaseOptions options) {
threadManager = options.threadManager;
connectTimeout = options.connectTimeout;
readTimeout = options.readTimeout;
writeTimeout = options.writeTimeout;
firestoreOptions = options.firestoreOptions;
}

Expand Down Expand Up @@ -495,6 +510,19 @@ public Builder setReadTimeout(int readTimeout) {
return this;
}

/**
* Sets the write timeout for outgoing HTTP (REST) calls made by the SDK. This does not affect
* the {@link com.google.firebase.database.FirebaseDatabase} and
* {@link com.google.firebase.cloud.FirestoreClient} APIs.
*
* @param writeTimeout Write timeout in milliseconds. Must not be negative.
* @return This <code>Builder</code> instance is returned so subsequent calls can be chained.
*/
public Builder setWriteTimeout(int writeTimeout) {
this.writeTimeout = writeTimeout;
return this;
}

/**
* Builds the {@link FirebaseOptions} instance from the previously set options.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,16 +60,19 @@ private static class TimeoutInitializer implements HttpRequestInitializer {

private final int connectTimeoutMillis;
private final int readTimeoutMillis;
private final int writeTimeoutMillis;

TimeoutInitializer(FirebaseOptions options) {
this.connectTimeoutMillis = options.getConnectTimeout();
this.readTimeoutMillis = options.getReadTimeout();
this.writeTimeoutMillis = options.getWriteTimeout();
}

@Override
public void initialize(HttpRequest request) {
request.setConnectTimeout(connectTimeoutMillis);
request.setReadTimeout(readTimeoutMillis);
request.setWriteTimeout(writeTimeoutMillis);
}
}
}
12 changes: 11 additions & 1 deletion src/test/java/com/google/firebase/FirebaseOptionsTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@

import org.junit.Test;

/**
/**
* Tests for {@link FirebaseOptions}.
*/
public class FirebaseOptionsTest {
Expand Down Expand Up @@ -87,6 +87,7 @@ public void createOptionsWithAllValuesSet() throws IOException {
.setThreadManager(MOCK_THREAD_MANAGER)
.setConnectTimeout(30000)
.setReadTimeout(60000)
.setWriteTimeout(90000)
.setFirestoreOptions(firestoreOptions)
.build();
assertEquals(FIREBASE_DB_URL, firebaseOptions.getDatabaseUrl());
Expand All @@ -97,6 +98,7 @@ public void createOptionsWithAllValuesSet() throws IOException {
assertSame(MOCK_THREAD_MANAGER, firebaseOptions.getThreadManager());
assertEquals(30000, firebaseOptions.getConnectTimeout());
assertEquals(60000, firebaseOptions.getReadTimeout());
assertEquals(90000, firebaseOptions.getWriteTimeout());
assertSame(firestoreOptions, firebaseOptions.getFirestoreOptions());

GoogleCredentials credentials = firebaseOptions.getCredentials();
Expand Down Expand Up @@ -209,6 +211,14 @@ public void createOptionsWithInvalidReadTimeout() {
.build();
}

@Test(expected = IllegalArgumentException.class)
public void createOptionsWithInvalidWriteTimeout() {
FirebaseOptions.builder()
.setCredentials(TestUtils.getCertCredential(ServiceAccount.EDITOR.asStream()))
.setWriteTimeout(-1)
.build();
}

@Test
public void testNotEquals() throws IOException {
GoogleCredentials credentials = GoogleCredentials.fromStream(ServiceAccount.EDITOR.asStream());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -979,6 +979,7 @@ public void testTimeout() throws Exception {
.setHttpTransport(transport)
.setConnectTimeout(30000)
.setReadTimeout(60000)
.setWriteTimeout(90000)
.build());
FirebaseAuth auth = FirebaseAuth.getInstance();
FirebaseUserManager userManager = auth.getUserManager();
Expand All @@ -989,6 +990,7 @@ public void testTimeout() throws Exception {
HttpRequest request = interceptor.getResponse().getRequest();
assertEquals(30000, request.getConnectTimeout());
assertEquals(60000, request.getReadTimeout());
assertEquals(90000, request.getWriteTimeout());
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ public class FirebaseRequestInitializerTest {
private static final int MAX_RETRIES = 5;
private static final int CONNECT_TIMEOUT_MILLIS = 30000;
private static final int READ_TIMEOUT_MILLIS = 60000;
private static final int WRITE_TIMEOUT_MILLIS = 90000;

@After
public void tearDown() {
Expand Down Expand Up @@ -69,6 +70,7 @@ public void testExplicitTimeouts() throws Exception {
.setCredentials(new MockGoogleCredentials("token"))
.setConnectTimeout(CONNECT_TIMEOUT_MILLIS)
.setReadTimeout(READ_TIMEOUT_MILLIS)
.setWriteTimeout(WRITE_TIMEOUT_MILLIS)
.build());
HttpRequest request = TestUtils.createRequest();

Expand All @@ -77,6 +79,7 @@ public void testExplicitTimeouts() throws Exception {

assertEquals(CONNECT_TIMEOUT_MILLIS, request.getConnectTimeout());
assertEquals(READ_TIMEOUT_MILLIS, request.getReadTimeout());
assertEquals(WRITE_TIMEOUT_MILLIS, request.getWriteTimeout());
assertEquals("Bearer token", request.getHeaders().getAuthorization());
assertEquals(HttpRequest.DEFAULT_NUMBER_OF_RETRIES, request.getNumberOfRetries());
assertNull(request.getIOExceptionHandler());
Expand Down