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

[JENKINS-45233] - Log errors when Response message cannot be delivered due to the closed channel #177

Merged
merged 4 commits into from Jul 3, 2017

Conversation

Projects
None yet
4 participants
@oleg-nenashev
Copy link
Member

commented Jun 30, 2017

Just noticed some issues in command execution logic...

  • JENKINS-45233 - Command processing does not print errors when the Response cannot be delivered due to the closed channel
  • Add FINE/FINER logging for better diagnostics of incoming command processing

@reviewbybees

@oleg-nenashev oleg-nenashev changed the title [JENKINS-45233] - [JENKINS-45233] - Log errors when Response message cannot be delivered due to the closed channel Jun 30, 2017

@reviewbybees

This comment has been minimized.

Copy link

commented Jun 30, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@jglick

jglick approved these changes Jun 30, 2017

channel.send(rsp);
} else {
// Propagate exception to the catch clause instead of supressing it

This comment has been minimized.

Copy link
@jglick
@@ -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()) {

This comment has been minimized.

Copy link
@jglick

jglick Jun 30, 2017

Member

arguably easier to read if test is inverted

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jun 30, 2017

Author Member

Actually the check is not required at all. Channel#send() does the check

channel.send(rsp);
} else {
// Propagate exception to the catch clause instead of supressing it
throw new ClosedChannelException();

This comment has been minimized.

Copy link
@jglick

jglick Jun 30, 2017

Member

Is there a known cause here?

}
} 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);

This comment has been minimized.

Copy link
@jglick

jglick Jun 30, 2017

Member

BTW SEVERE is probably excessive.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jun 30, 2017

Author Member

Yeah, I would also drop it to WARNING

@jtnord

This comment has been minimized.

Copy link
Member

commented Jun 30, 2017

Command does not implement toString() - yet it is used in lots of logging - are you intentionally wanting to print the hashCode - in which case I would suggest you replace it's usage with System.identityHashCode(it)
if you are expecting somethign else then shouldn't you make toString() abstract and force its implementation according to a spec (defined in the classes javadoc)?

@@ -346,13 +347,12 @@ public void run() {
rsp.createdAt.initCause(createdAt);

synchronized (channel) {// expand the synchronization block of the send() method to a check

This comment has been minimized.

Copy link
@jtnord

jtnord Jun 30, 2017

Member

comment implies this should be removed.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jun 30, 2017

Author Member

right

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

commented Jun 30, 2017

Command does not implement toString() - yet it is used in lots of logging - are you intentionally wanting to print the hashCode - in which case I would suggest you replace it's usage with System.identityHashCode(it)
if you are expecting somethign else then shouldn't you make toString() abstract and force its implementation according to a spec (defined in the classes javadoc)?

Implementations like UserRequest and RPCRequest have toString() implementations, hence I do not consider it as a big problem. But yes, it will be printing class names and hashes for system calls. Not a big deal

@jtnord

jtnord approved these changes Jun 30, 2017

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

commented Jun 30, 2017

@jglick

jglick approved these changes Jun 30, 2017

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2017

Errors are unrelated

@oleg-nenashev oleg-nenashev merged commit 47b20da into jenkinsci:master Jul 3, 2017

0 of 2 checks passed

Jenkins
Details
continuous-integration/jenkins/pr-head This commit cannot be built
Details

@oleg-nenashev oleg-nenashev deleted the oleg-nenashev:bug/JENKINS-45233 branch Jul 3, 2017

oleg-nenashev added a commit to oleg-nenashev/remoting that referenced this pull request Sep 26, 2017

[JENKINS-45233] - Log errors when Response message cannot be delivere…
…d due to the closed channel (jenkinsci#177)

* [FIXED JENKINS-45223] - Print warnings about failed responses when cannot deliver them due to the closed channel

* Improve logging of the command execution

* [JENKINS-45233] - Address comments from @jglick

* [JENKINS-45233] - get rid of gratituos synchronization
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.