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.

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

[JENKINS-34819] Allow disabling the protocols individually #83

Merged
merged 1 commit into from
May 12, 2016
Merged

[JENKINS-34819] Allow disabling the protocols individually #83

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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 src/main/java/hudson/remoting/Engine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,12 +302,24 @@ public void run() {
Channel channel = null;

// Try available protocols.
boolean triedAtLeastOneProtocol = false;
for (JnlpProtocol protocol : protocols) {
if (!protocol.isEnabled()) {
events.status("Protocol " + protocol.getName() + " is not enabled, skipping");
continue;
}
triedAtLeastOneProtocol = true;
events.status("Trying protocol: " + protocol.getName());
try {
channel = protocol.establishChannel(jnlpSocket, channelBuilder);
} catch (IOException ioe) {
events.status("Protocol failed to establish channel", ioe);
events.status("Protocol " + protocol.getName() + " failed to establish channel", ioe);
} catch (RuntimeException e) {
events.status("Protocol " + protocol.getName() + " encountered a runtime error", e);
} catch (Error e) {
events.status("Protocol " + protocol.getName() + " could not be completed due to an error", e);
Copy link
Member

@oleg-nenashev oleg-nenashev May 12, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Uhm. Do we really want to continue after it? Errors are commonly fatal

Copy link
Member Author

Choose a reason for hiding this comment

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

IOError is one that we should continue after and I fear could be one that gets thrown

Copy link
Member Author

Choose a reason for hiding this comment

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

feel free to update this PR to whatever you want, but if JnlpProtocol3 throws an Error, e.g. LinkerError, etc we should still try the others

Copy link
Member

Choose a reason for hiding this comment

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

I think it's acceptable. If the error is not handleable, it will just blow up in another place

} catch (Throwable e) {
events.status("Protocol " + protocol.getName() + " encountered an unexpected exception", e);
}

// On success do not try other protocols.
Expand All @@ -322,7 +334,12 @@ public void run() {

// If no protocol worked.
if (channel == null) {
onConnectionRejected("None of the protocols were accepted");
if (triedAtLeastOneProtocol) {
onConnectionRejected("None of the protocols were accepted");
} else {
onConnectionRejected("None of the protocols are enabled");
return; // exit
}
continue;
}

Expand Down
8 changes: 8 additions & 0 deletions src/main/java/org/jenkinsci/remoting/engine/JnlpProtocol.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,14 @@ public abstract class JnlpProtocol {
this.events = events;
}

/**
* Whether this protocol is enabled for connecting.
* @return {@code true} if this protocol is enabled.
*/
public boolean isEnabled() {
return !Boolean.getBoolean(getClass().getName()+".disabled");
Copy link
Member

Choose a reason for hiding this comment

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

so we retrieve the property every time. Useful for debugging, performance should not suffer in this place

}

/**
* Get the name of the protocol.
*/
Expand Down