-
-
Notifications
You must be signed in to change notification settings - Fork 272
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
[JENKINS-45233] - Log errors when Response message cannot be delivered due to the closed channel #177
[JENKINS-45233] - Log errors when Response message cannot be delivered due to the closed channel #177
Changes from 2 commits
e34a325
826d037
6d339ef
e5caf39
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,6 +25,7 @@ | |
|
||
import java.io.IOException; | ||
import java.io.Serializable; | ||
import java.nio.channels.ClosedChannelException; | ||
import java.util.concurrent.CancellationException; | ||
import java.util.concurrent.ExecutionException; | ||
import java.util.concurrent.Future; | ||
|
@@ -346,13 +347,17 @@ public void run() { | |
rsp.createdAt.initCause(createdAt); | ||
|
||
synchronized (channel) {// expand the synchronization block of the send() method to a check | ||
if(!channel.isOutClosed()) | ||
if(!channel.isOutClosed()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. arguably easier to read if test is inverted There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually the check is not required at all. |
||
channel.send(rsp); | ||
} else { | ||
// Propagate exception to the catch clause instead of supressing it | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo |
||
throw new ClosedChannelException(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a known |
||
} | ||
} | ||
} catch (IOException e) { | ||
// communication error. | ||
// this means the caller will block forever | ||
logger.log(Level.SEVERE, "Failed to send back a reply",e); | ||
logger.log(Level.SEVERE, "Failed to send back a reply to the request " + this, e); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BTW There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I would also drop it to WARNING |
||
} finally { | ||
channel.executingCalls.remove(id); | ||
Thread.currentThread().setName(oldThreadName); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment implies this should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right