From 04e7974825d1226309e0732a5084e36a72accaf5 Mon Sep 17 00:00:00 2001 From: Marc Khouzam Date: Tue, 10 May 2011 17:38:42 +0000 Subject: [PATCH] Bug 345164: Revert changes to Spawner and instead explicitly close streams in DSF-GDB and CDI --- .../eclipse/cdt/utils/spawner/Spawner.java | 81 ++++++++++++++----- .../cdt/debug/mi/core/MIProcessAdapter.java | 13 +++ .../cdt/dsf/gdb/launching/LaunchUtils.java | 9 ++- .../cdt/dsf/gdb/service/GDBBackend.java | 12 ++- 4 files changed, 91 insertions(+), 24 deletions(-) diff --git a/core/org.eclipse.cdt.core/utils/org/eclipse/cdt/utils/spawner/Spawner.java b/core/org.eclipse.cdt.core/utils/org/eclipse/cdt/utils/spawner/Spawner.java index 3a8bff6e322..05f5098c41d 100644 --- a/core/org.eclipse.cdt.core/utils/org/eclipse/cdt/utils/spawner/Spawner.java +++ b/core/org.eclipse.cdt.core/utils/org/eclipse/cdt/utils/spawner/Spawner.java @@ -143,6 +143,7 @@ public class Spawner extends Process { /** * See java.lang.Process#getInputStream (); + * The client is responsible for closing the stream explicitly. **/ @Override public InputStream getInputStream() { @@ -158,6 +159,7 @@ public class Spawner extends Process { /** * See java.lang.Process#getOutputStream (); + * The client is responsible for closing the stream explicitly. **/ @Override public OutputStream getOutputStream() { @@ -173,6 +175,7 @@ public class Spawner extends Process { /** * See java.lang.Process#getErrorStream (); + * The client is responsible for closing the stream explicitly. **/ @Override public InputStream getErrorStream() { @@ -202,21 +205,28 @@ public class Spawner extends Process { while (!isDone) { wait(); } - // Close the streams on this side. - // If the stream was never used - // we still want to create it, so that we can close - // the pipe on the other side. + + // For situations where the user does not call destroy(), + // we try to kill the streams that were not used here. + // We check for streams that were not created, we create + // them to attach to the pipes, and then we close them + // to release the pipes. + // Streams that were created by the client need to be + // closed by the client itself. // - // Technically, we shouldn't close the streams here but - // should rely on destroy() being called properly. - // However, since we did close the streams here, we - // cannot remove this code or we would break anyone who - // does not call destroy() properly and relies on this - // code instead. - // Bug 345164 - try { getErrorStream().close(); } catch (IOException e) {} - try { getInputStream().close(); } catch (IOException e) {} - try { getOutputStream().close();} catch (IOException e) {} + // But 345164 + try { + if(null == err) + getErrorStream().close(); + } catch (IOException e) {} + try { + if(null == in) + getInputStream().close(); + } catch (IOException e) {} + try { + if(null == out) + getOutputStream().close(); + } catch (IOException e) {} return status; } @@ -233,20 +243,47 @@ public class Spawner extends Process { /** * See java.lang.Process#destroy (); + * + * Clients are responsible for explicitly closing any streams + * that they have requested through + * getErrorStream(), getInputStream() or getOutputStream() **/ @Override public synchronized void destroy() { // Sends the TERM terminate(); + // Close the streams on this side. - // If the stream was never used - // we still want to create it, so that we can close - // the pipe on the other side. - // Bug 345164 - try { getErrorStream().close(); } catch (IOException e) {} - try { getInputStream().close(); } catch (IOException e) {} - try { getOutputStream().close();} catch (IOException e) {} - + // + // We only close the streams that were + // never used by any client. + // So, if the stream was not created yet, + // we create it ourselves and close it + // right away, so as to release the pipe. + // Note that even if the stream was never + // created, the pipe has been allocated in + // native code, so we need to create the + // stream and explicitly close it. + // + // We don't close streams the clients have + // created because we don't know when the + // client will be finished using them. + // It is up to the client to close those + // streams. + // + // But 345164 + try { + if(null == err) + getErrorStream().close(); + } catch (IOException e) {} + try { + if(null == in) + getInputStream().close(); + } catch (IOException e) {} + try { + if(null == out) + getOutputStream().close(); + } catch (IOException e) {} // Grace before using the heavy gone. if (!isDone) { try { diff --git a/debug/org.eclipse.cdt.debug.mi.core/src/org/eclipse/cdt/debug/mi/core/MIProcessAdapter.java b/debug/org.eclipse.cdt.debug.mi.core/src/org/eclipse/cdt/debug/mi/core/MIProcessAdapter.java index b0d446a7e26..f504ba5b5eb 100644 --- a/debug/org.eclipse.cdt.debug.mi.core/src/org/eclipse/cdt/debug/mi/core/MIProcessAdapter.java +++ b/debug/org.eclipse.cdt.debug.mi.core/src/org/eclipse/cdt/debug/mi/core/MIProcessAdapter.java @@ -172,6 +172,19 @@ public class MIProcessAdapter implements MIProcess { } public void destroy() { + // We are responsible for closing the streams we have used or else + // we will leak pipes. + // Bug 345164 + try { + getErrorStream().close(); + } catch (IOException e) {} + try { + getInputStream().close(); + } catch (IOException e) {} + try { + getOutputStream().close(); + } catch (IOException e) {} + fGDBProcess.destroy(); } diff --git a/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/launching/LaunchUtils.java b/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/launching/LaunchUtils.java index 29d2a1b911a..e1dfd88601d 100644 --- a/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/launching/LaunchUtils.java +++ b/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/launching/LaunchUtils.java @@ -301,9 +301,10 @@ public class LaunchUtils { "Error while launching command: " + cmd, e.getCause()));//$NON-NLS-1$ } + InputStream stream = null; StringBuilder cmdOutput = new StringBuilder(200); try { - InputStream stream = process.getInputStream(); + stream = process.getInputStream(); Reader r = new InputStreamReader(stream); BufferedReader reader = new BufferedReader(r); @@ -316,7 +317,13 @@ public class LaunchUtils { "Error reading GDB STDOUT after sending: " + cmd, e.getCause()));//$NON-NLS-1$ } finally { // Cleanup to avoid leaking pipes + // Close the stream we used, and then destroy the process // Bug 345164 + if (stream != null) { + try { + stream.close(); + } catch (IOException e) {} + } process.destroy(); } diff --git a/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/GDBBackend.java b/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/GDBBackend.java index 4c8659aaa52..d1232f9389e 100644 --- a/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/GDBBackend.java +++ b/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/GDBBackend.java @@ -442,7 +442,17 @@ public class GDBBackend extends AbstractDsfService implements IGDBBackend { } public void destroy() { - // destroy() should be supported even if it's not spawner. + // We are responsible for closing the streams we have used or else + // we will leak pipes. + // Bug 345164 + try { + getMIOutputStream().close(); + } catch (IOException e) {} + try { + getMIInputStream().close(); + } catch (IOException e) {} + + // destroy() should be supported even if it's not spawner. if (getState() == State.STARTED) { fProcess.destroy(); }