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

Jump to bottom

Do not use synchronised checks during event processing #363

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Do not use synchronised checks during event processing #363

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d50f135
Do not use synchronised checks during event processing
Jimilian May 16, 2018
37b2b42
Remove synchronization for onTriggered method
Jimilian May 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 6 additions & 7 deletions ...va/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/ToGerritRunListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,11 @@ public synchronized void allBuildsCompleted(GerritTriggeredEvent event, GerritCa
* @param event the Gerrit Event which is being checked.
* @param p the Gerrit project being checked.
* @return true if so.
*
* @deprecated Use {@link #isTriggered(Job, GerritTriggeredEvent)} and
* {@link #isBuilding(Job, GerritTriggeredEvent)}} instead
*/
@Deprecated
public synchronized boolean isProjectTriggeredAndIncomplete(Job p, GerritTriggeredEvent event) {
if (!memory.isTriggered(event, p)) {
return false;
Expand Down Expand Up @@ -328,18 +332,13 @@ protected void cleanUpGerritCauses(GerritCause firstFound, Run build) {
* @param project the project that will be built.
* @param event the event that caused the build to be scheduled.
*/
public synchronized void onTriggered(Job project, GerritTriggeredEvent event) {
public void onTriggered(Job project, GerritTriggeredEvent event) {
//TODO stop builds for earlier patch-sets on same change.
memory.triggered(event, project);
if (event instanceof GerritEventLifecycle) {
Copy link
Member

Choose a reason for hiding this comment

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

This is a change that might have outside repercussions. The lifecycle listeners might have assumptions about owning the monitor when called (which is normal behavior in most listener cases that I've seen)
But I think I can live with that risk if this change is really needed.

((GerritEventLifecycle)event).fireProjectTriggered(project);
}
//Logging
String name = null;
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove this?

if (project != null) {
name = project.getName();
}
logger.info("Project [{}] triggered by Gerrit: [{}]", name, event);
logger.info("Project [{}] triggered by Gerrit: [{}]", project.getName(), event);
}

/**
Expand Down
11 changes: 3 additions & 8 deletions ...main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/GerritTrigger.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -949,14 +949,9 @@ public boolean isInteresting(GerritTriggeredEvent event) {
}

ToGerritRunListener listener = ToGerritRunListener.getInstance();
if (listener != null) {
if (listener.isProjectTriggeredAndIncomplete(job, event)) {
logger.trace("Already triggered and incomplete.");
return false;
} else if (listener.isTriggered(job, event)) {
logger.trace("Already triggered.");
return false;
}
if (listener != null && listener.isTriggered(job, event)) {
logger.trace("{} is already triggered for the event {}.", job, event);
return false;
}

if (!shouldTriggerOnEventType(event)) {
Expand Down