Skip to content
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

Remove incorrect assertions from logging lifecycle observers #2644

Merged
merged 1 commit into from
Jul 6, 2023
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
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,6 @@ private LoggingGrpcExchangeObserver(final FixedLevelLogger logger) {

@Override
public void onConnectionSelected(final ConnectionInfo info) {
assert this.connInfo == null;
this.connInfo = info;
}

Expand All @@ -111,20 +110,19 @@ public void onRequestTrailers(final HttpHeaders trailers) {

@Override
public void onRequestComplete() {
assert requestResult == null;
assert requestMetaData != null;
assert requestMetaData != null : "Request meta-data is not expected to be null on completion";
requestResult = Result.complete;
}

@Override
public void onRequestError(final Throwable cause) {
assert requestResult == null;
assert requestMetaData != null : "Request meta-data is not expected to be null on error";
requestResult = cause;
}

@Override
public void onRequestCancel() {
assert requestResult == null;
assert requestMetaData != null : "Request meta-data is not expected to be null on cancel";
requestResult = Result.cancelled;
}

Expand All @@ -151,28 +149,24 @@ public void onResponseTrailers(final HttpHeaders trailers) {

@Override
public void onGrpcStatus(final GrpcStatus status) {
assert this.grpcStatus == null;
this.grpcStatus = status.code();
}

@Override
public void onResponseComplete() {
assert responseResult == null;
assert responseMetaData != null;
assert responseMetaData != null : "Response meta-data is not expected to be null on completion";
responseTimeMs = durationMs(startTime);
responseResult = Result.complete;
}

@Override
public void onResponseError(final Throwable cause) {
assert responseResult == null;
responseTimeMs = durationMs(startTime);
responseResult = cause;
}

@Override
public void onResponseCancel() {
assert responseResult == null;
responseTimeMs = durationMs(startTime);
responseResult = Result.cancelled;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ public interface HttpLifecycleObserver {

/**
* Callback when a new HTTP exchange starts.
* <p>
* Depending on the order in which the observer is applied, this callback can be invoked either for every retry
* attempt (if an observer is added after retrying filter) or only once per exchange.
*
* @return an {@link HttpExchangeObserver} that provides visibility into exchange events
*/
Expand All @@ -56,6 +59,8 @@ interface HttpExchangeObserver {

/**
* Callback when a connection is selected for this exchange execution.
* <p>
* This callback may be invoked for every retry attempt.
*
* @param info {@link ConnectionInfo} of the selected connection
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
import io.servicetalk.logging.slf4j.internal.FixedLevelLogger;
import io.servicetalk.transport.api.ConnectionInfo;

import java.util.Arrays;
import javax.annotation.Nullable;

import static io.servicetalk.logging.slf4j.internal.Slf4jFixedLevelLoggers.newLogger;
Expand Down Expand Up @@ -81,7 +80,6 @@ private LoggingHttpExchangeObserver(final FixedLevelLogger logger) {

@Override
public void onConnectionSelected(final ConnectionInfo info) {
assert this.connInfo == null;
this.connInfo = info;
}

Expand All @@ -108,23 +106,19 @@ public void onRequestTrailers(final HttpHeaders trailers) {

@Override
public void onRequestComplete() {
Object current = requestResult;
assert current == null : assertResultMsg(current, "requestResult");
assert requestMetaData != null : "Request meta-data is not expected to be null on completion";
requestResult = Result.complete;
}

@Override
public void onRequestError(final Throwable cause) {
Object current = requestResult;
assert current == null : assertResultMsg(current, "requestResult");
assert requestMetaData != null : "Request meta-data is not expected to be null on error";
requestResult = cause;
}

@Override
public void onRequestCancel() {
Object current = requestResult;
assert current == null : assertResultMsg(current, "requestResult");
assert requestMetaData != null : "Request meta-data is not expected to be null on cancel";
requestResult = Result.cancelled;
}

Expand All @@ -151,25 +145,19 @@ public void onResponseTrailers(final HttpHeaders trailers) {

@Override
public void onResponseComplete() {
Object current = responseResult;
assert current == null : assertResultMsg(current, "responseResult");
assert responseMetaData != null;
assert responseMetaData != null : "Response meta-data is not expected to be null on completion";
responseTimeMs = durationMs(startTime);
responseResult = Result.complete;
}

@Override
public void onResponseError(final Throwable cause) {
Object current = responseResult;
assert current == null : assertResultMsg(current, "responseResult");
responseTimeMs = durationMs(startTime);
responseResult = cause;
}

@Override
public void onResponseCancel() {
Object current = responseResult;
assert current == null : assertResultMsg(current, "responseResult");
responseTimeMs = durationMs(startTime);
responseResult = Result.cancelled;
}
Expand Down Expand Up @@ -221,11 +209,5 @@ private static long durationMs(final long startTime) {
private enum Result {
complete, error, cancelled
}

private static String assertResultMsg(final Object current, final String name) {
return "Unexpected " + name + ": " + current + (current instanceof Throwable ?
'\n' + String.join("\n", Arrays.stream(((Throwable) current).getStackTrace())
.map(String::valueOf).toArray(CharSequence[]::new)) : "");
}
}
}