Skip to content

[Java] Observe error to avoid crash #22016

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.

Sign up for GitHub

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
Jun 9, 2020
Merged

[Java] Observe error to avoid crash #22016

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
16b8ae6
[Java] Observe error to avoid crash
BrennanConroy Apr 2, 2020
9d19674
avoid calling onError on completed subject
BrennanConroy Jun 3, 2020
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
21 changes: 19 additions & 2 deletions ...nalR/clients/java/signalr/src/main/java/com/microsoft/signalr/OkHttpWebSocketWrapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package com.microsoft.signalr;

import java.util.Map;
import java.util.concurrent.locks.ReentrantLock;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand All @@ -26,6 +27,7 @@ class OkHttpWebSocketWrapper extends WebSocketWrapper {
private WebSocketOnClosedCallback onClose;
private CompletableSubject startSubject = CompletableSubject.create();
private CompletableSubject closeSubject = CompletableSubject.create();
private final ReentrantLock closeLock = new ReentrantLock();

private final Logger logger = LoggerFactory.getLogger(OkHttpWebSocketWrapper.class);

Expand Down Expand Up @@ -87,14 +89,29 @@ public void onMessage(WebSocket webSocket, String message) {
@Override
public void onClosing(WebSocket webSocket, int code, String reason) {
onClose.invoke(code, reason);
closeSubject.onComplete();
try {
closeLock.lock();
closeSubject.onComplete();
Copy link
Member

@halter73 halter73 Jun 4, 2020

Choose a reason for hiding this comment

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

Should we check that the closeSubject hasn't already been completed with an error here?

Copy link
Member Author

@BrennanConroy BrennanConroy Jun 4, 2020

Choose a reason for hiding this comment

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

Calling onComplete should noop if the subject is already completed either with an error or without an error.

Copy link
Member Author

@BrennanConroy BrennanConroy Jun 9, 2020

Choose a reason for hiding this comment

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

@halter73 any other feedback?

}
finally {
closeLock.unlock();
}
checkStartFailure();
}

@Override
public void onFailure(WebSocket webSocket, Throwable t, Response response) {
logger.error("WebSocket closed from an error: {}.", t.getMessage());
closeSubject.onError(new RuntimeException(t));

try {
closeLock.lock();
if (!closeSubject.hasComplete()) {
closeSubject.onError(new RuntimeException(t));
}
}
finally {
closeLock.unlock();
}
onClose.invoke(null, t.getMessage());
checkStartFailure();
Copy link
Member

@halter73 halter73 May 20, 2020

Choose a reason for hiding this comment

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

Is there a chance that checkStartFailure() ever gets called before the Completable is returned from start() meaning we'd need to do the same ting for startSubject?

Copy link
Member Author

@BrennanConroy BrennanConroy May 21, 2020

Choose a reason for hiding this comment

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

I think that is technically possible, but very unlikely in practice.

I'm not sure how the global error handler works in that case, because the startSubject will be observed by someone, they just haven't registered the handler by the time onError is called.

Copy link
Member

@halter73 halter73 May 21, 2020

Choose a reason for hiding this comment

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

If it's possible, we should handle it.

I'm not sure how the global error handler works in that case, because the startSubject will be observed by someone, they just haven't registered the handler by the time onError is called.

We could test this by intentionally faulting the startSubject before returning. I expect it would also be crashing. How would the runtime even know the startSubject will be observed by someone in the future?

Copy link
Member Author

@BrennanConroy BrennanConroy Jun 3, 2020

Choose a reason for hiding this comment

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

Read the docs for CompletableSubject:

If there were no CompletableObservers subscribed to this CompletableSubject when the onError() was called, the global error handler is not invoked.

This also implies that the "fix" in this PR might not be correct, and the error was happening through some other means. The docs also mention that if onError is called multiple times then it will call the global handler. onError should only ever be called once here, but I'm beginning to think it was somehow triggered multiple times.

}
Expand Down