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

[NETBEANS-2038] Do not dump ambiguous output when a build is cancelled. #1138

Conversation

lkishalmi
Copy link
Contributor

No description provided.

@lkishalmi lkishalmi added the Cherry Pick Consider cherry picking for release update label Feb 17, 2019
Copy link
Contributor

@jlahoda jlahoda left a comment

Choose a reason for hiding this comment

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

Overall seems fine to me, but I wonder if the access to the newly added fields should be synchronized, or something like that.

@@ -69,6 +69,9 @@
private static final Logger LOGGER = Logger.getLogger(GradleDaemonExecutor.class.getName());

private final ProgressHandle handle;
private OutputStream outStream;
Copy link
Contributor

Choose a reason for hiding this comment

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

So, assuming the cancel can be asynchronous, I wonder if there's a need for synchronization for these fields?

// This can happen if cancelling a Gradle build which is running
// an external aplication
showAbort();
}
//TODO: Handle Cancelled builds
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: these comments are probably obsolete now?

boolean esc;
boolean csi;
boolean closed = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if some kind of synchronization is needed for the field? (Maybe AtomicBoolean instead of plain boolean?)

@lkishalmi lkishalmi force-pushed the NETBEANS-2038_Anomalous_termination_of_gradle_task branch from 07bb600 to 011f78f Compare February 17, 2019 18:13
@lkishalmi
Copy link
Contributor Author

@jlahoda Thanks for the review. You are right it is better to be safe than sorry. I've applied the requested changes.

@lkishalmi lkishalmi merged commit e8db731 into apache:master Feb 17, 2019
@lkishalmi lkishalmi removed the Cherry Pick Consider cherry picking for release update label Feb 17, 2019
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.

2 participants