Skip to content
Open
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
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,7 @@ private void connect() {
}
} else {
logger.debug("Unsuccessful Response: " + response);
errorHandlerAction = dispatchError(new UnsuccessfulResponseException(response.code()));
errorHandlerAction = dispatchError(new UnsuccessfulResponseException(response));
}
} catch (EOFException eofe) {
logger.warn("Connection unexpectedly closed.");
Expand Down
Original file line number Diff line number Diff line change
@@ -1,27 +1,64 @@
package com.launchdarkly.eventsource;

import okhttp3.Protocol;
import okhttp3.Request;
import okhttp3.Response;

/**
* Exception class that means the remote server returned an HTTP error.
*/
@SuppressWarnings("serial")
public class UnsuccessfulResponseException extends Exception {

private final int code;
private final Response response;

/**
* Constructs an exception instance.
* @param code the HTTP status
*/
@Deprecated
public UnsuccessfulResponseException(int code) {
super("Unsuccessful response code received from stream: " + code);
this.code = code;
this(buildSurrogateResponse(code));
Copy link
Contributor

Choose a reason for hiding this comment

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

I would not do this; it'd be preferable to just let getResponse() return null in this case. The rationale can't be backward compatibility, since old code that uses this deprecated constructor couldn't be looking for a Response.

Copy link
Author

Choose a reason for hiding this comment

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

Actually, I also realized that the whole PR didn't work.

I think due to the async fact that the whole response is not available yet when UnsuccessfulResponseException is created within the library's code (unlike the error code).
In other words, there are more changes needed (even if this constructor change is reverted).

So far, we happily used the library without this change anyway - @eli-darkly , I'm OK if it is rejected (when I need it badly, I'll propose proper change).

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. Can you say more about what specifically was not working? I would expect that trying to read the body would not work, since other code might already have read the body - that's just an expected issue whenever you share a Response around. But I would have expected the headers to be available.

}

/**
* Constructs an exception instance.
* @param response {@link Response} provided by server
*/
public UnsuccessfulResponseException(Response response) {
super("Unsuccessful response code received from stream: " + response.code());
this.response = response;
}

/**
* Returns the HTTP status code.
* @return the HTTP status
*/
public int getCode() {
return code;
return response.code();
}

/**
* Returns the HTTP {@link Response}.
* @return the HTTP {@link Response}
*/
public Response getResponse() {
return response;
}

/**
* Used for backward compatibility only: provide surrogate response for a given code
*/
@Deprecated
private static Response buildSurrogateResponse(int code) {
return new Response.Builder()
.request(new Request.Builder()
.url("http://example.com")
.build()
)
.protocol(Protocol.HTTP_1_1)
.message("")
.code(code)
.build();
}
}