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-63164] Clear fields in CpsThread when thread is removed from CpsThreadGroup #368

Merged
merged 3 commits into from
Jul 24, 2020

Conversation

dwnusbaum
Copy link
Member

See #367. This is the first alternative discussed in #367 (comment).

@@ -419,6 +419,7 @@ private boolean run() {
t.fireCompletionHandlers(o); // do this after ErrorAction is set above

threads.remove(t.id);
t.cleanUp();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I like this better than #367 because it seems like there's fewer paths in which this call could be missed but I might not be understanding.

Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

I cannot say that this is right, either, but at least it strikes me as more intuitive.

@dwnusbaum
Copy link
Member Author

@bitwiseman I added some assertions for CpsThread.program in fa8bd21 like you requested. As far as I can tell, that field previously could have been final and marked as non-null, so I'm not totally sure why the assertion in CpsThread.run existed. For existing code, program is only accessed when a live CpsThread is accessed through a CpsThreadGroup, so the assertions should never be triggered.

The other fields we clean up are all nullable, so it doesn't really make sense to add assertions around them.

@dwnusbaum dwnusbaum merged commit e405d49 into jenkinsci:master Jul 24, 2020
@dwnusbaum dwnusbaum deleted the JENKINS-63164-2 branch July 24, 2020 19:00
@dwnusbaum dwnusbaum mentioned this pull request Jul 30, 2020
dwnusbaum added a commit to dwnusbaum/workflow-cps-plugin that referenced this pull request Aug 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants