Skip to content

[401] stop piper thread via flag #403

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

Merged
merged 2 commits into from
Jan 29, 2025
Merged

Conversation

ghentschke
Copy link
Contributor

fixes #401

Copy link
Member

@jonahgraham jonahgraham left a comment

Choose a reason for hiding this comment

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

I can't add line comments because the lines I want to comment on are unmodified:

For:

final Runnable c = cancelation[0];
if ((c != null)) {
	synchronized (c) {
		c.notify();
	}
}

I don't think this is needed anymore as I don't think anything is waiting on Runnable c. Once you do that, I think a bit of simplification is trivally possible as final Runnable[] cancelation is no longer needed and the method can just return stopper.

For:

} catch (Throwable t) {
	throw sneakyThrow(t);
}

I think this needs to check for stop and suppress the exception if stop is requested. This is because at the point that stop is requested the code may be either inside of the read() call or just after you checked for stop but before read is called. In that case you want to suppress the now expected exception. E.g:

} catch (Throwable t) {
	if (!stop.get()) {
		throw sneakyThrow(t);
	}
}

This structure I am recommending here is based on OutputStremMonitor (link to it's catch block)

@@ -65,8 +65,8 @@ protected ProcessBuilder createProcessBuilder() {
public void start() throws IOException {
super.start();
if (logEnabled() && getLogProvider().isPresent()) {
errorStreamPipeStopper = new AsyncStreamPipe().pipeTo(new BufferedInputStream(getErrorStream()),
getLogProvider().get().getOutputStream());
errorStreamPipeStopper = new AsyncStreamPipe().pipeTo("LS stderr pipe", //$NON-NLS-1$
Copy link
Member

Choose a reason for hiding this comment

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

I like that you added a thread name. Can you mention CDT in it?

Suggested change
errorStreamPipeStopper = new AsyncStreamPipe().pipeTo("LS stderr pipe", //$NON-NLS-1$
errorStreamPipeStopper = new AsyncStreamPipe().pipeTo("CDT LS stderr pipe", //$NON-NLS-1$

@ghentschke
Copy link
Contributor Author

I think I fixed all issues mentioned above

@jonahgraham
Copy link
Member

There is a small amount of dead code still in this patchset hidden by the @SuppressWarnings("all") which isn't needed anymore. Specifically output cannot be null where it is checked in the catch block. OutputStremMonitor can have some nulls in the loop because it uses non-final fields instead of final locals.

@jonahgraham
Copy link
Member

BTW this last review is of very much diminishing returns, the current code operates correctly, just has some dead code.

I also accidentally pushed a change to your branch and then removed it:

image

@ghentschke ghentschke merged commit 4c3c448 into eclipse-cdt:main Jan 29, 2025
3 checks passed
@ghentschke ghentschke deleted the fix-401 branch January 29, 2025 06:15
@jonahgraham jonahgraham added this to the 3.0.0 milestone Mar 7, 2025
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.

IOExceptions thrown when clangd exits and clangd console enabled
2 participants