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

Fix NPE in case of race condition #110

Merged
merged 3 commits into from
Jan 22, 2017
Merged

Conversation

Jimilian
Copy link
Contributor

@Jimilian Jimilian commented Dec 23, 2016

NPE happens in rare case:

  1. We have X projects
  2. We disabled one of them
  3. Trigger fired X-1 projects and filled map <projectName, executor>
  4. Job was fixed, and we enable this job again
  5. Trigger is waiting for all projects (now it's X projects again)
  6. It tries to access job that we re-enabled -> it doesn't exist in map => NPE

How it looks like in logs:

Waiting for the completion of projectA
projectA #22344 completed. Result was SUCCESS
Waiting for the completion of projectB
ERROR: Build step failed with exception
java.lang.NullPointerException
Build step 'Trigger/call builds on other projects' marked build as failure

I failed to create test case for it :( .Because those two lines are too close to each other:

ListMultimap<Job, Future<Run>> futures = config.perform3(build, launcher, listener); // contains config.getJobs as well.
List<Job> projectList = config.getJobs(build.getRootBuild().getProject().getParent(), env);

In perfect world I would prefer different solution - take list of projects and provide this list to perform3 step. But it needs much more changes and usually maintainers are not so happy to merge so complicated stuff :)

@Jimilian
Copy link
Contributor Author

@olivergondza, please, review :)

@olivergondza
Copy link
Member

I failed to create test case for it :( .Because those two lines are too close to each other:

I do not follow. It should be possible to test the trigger build have not failed after downstream was re-enabled (and NPE is not logged).

} else {
buildStepResult = false;
listener.getLogger().println("Skipping " + HyperlinkNote.encodeTo('/'+ p.getUrl(), p.getFullDisplayName()) + ". The project was disabled.");
Copy link
Member

@oleg-nenashev oleg-nenashev Dec 27, 2016

Choose a reason for hiding this comment

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

It's only one cause of the potential null futures IIRC

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any suggestions what to put?
"The project was disabled or something went wrong"? :)

Copy link
Member

@oleg-nenashev oleg-nenashev Dec 27, 2016

Choose a reason for hiding this comment

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

Several cases:

  • perform3 Javadoc: // When a project is disabled or the configuration is not yet saved f will always be null and we're caught in a loop, therefore we need to check for it
  • BuildTriggerConfig#schedule: job is not an instance of ParameterizedJobMixIn.ParameterizedJob
  • ParameterizedJobMixIn#schedule2() returns null due to whatever other reason: Job is disabled, job is not buildable (e.g. triggering of Matrix Configuration), or Jenkins just does not like you (dispatcher extensions return null) :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can put The project was not triggered by some reason or

The project was not triggered because one of possible cases happened: 
1) Project was disabled in moment then this build started 
2) Configuration of that project was not saved then this build started 
3) Project is not instance of "ParameterizedJobMixIn.ParameterizedJob"
4) Jenkins just does not like you (dispatcher extensions return null instead of running this project) :(

Or keep only first part Skipping....

Copy link
Member

Choose a reason for hiding this comment

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

"The project was not triggered by some reason" is fine for me. It's rather an RFE to Jenkins core in order to get a good CauseOfRejection as a response

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@Jimilian
Copy link
Contributor Author

Jimilian commented Jan 2, 2017

@oleg-nenashev, @olivergondza, can we merge this one as well? pleeeeaaaase :)

@Jimilian
Copy link
Contributor Author

@oleg-nenashev, @kwhetstone, @olivergondza can we merge it, please?

@olivergondza olivergondza merged commit b6e1e63 into jenkinsci:master Jan 22, 2017
@Jimilian Jimilian deleted the fix_npe branch January 23, 2017 08:46
@Jimilian
Copy link
Contributor Author

@olivergondza Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants