Bugzilla – Full Text Bug Listing
| Summary: | Spawner streams are not being closed by debug client | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Marc Khouzam <marc.khouzam> | ||||||||||||||||
| Component: | cdt-debug | Assignee: | Marc Khouzam <marc.khouzam> | ||||||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | Ken Ryall <ken.ryall> | ||||||||||||||||
| Severity: | normal | ||||||||||||||||||
| Priority: | P3 | CC: | aleherb+eclipse, cdtdoug, elaskavaia.cdt, g.watson, jamesblackburn+eclipse, nobody, pawel.1.piech | ||||||||||||||||
| Version: | 8.0 | Flags: | jamesblackburn+eclipse:
review+
aleherb+eclipse: review+ |
||||||||||||||||
| Target Milestone: | 8.0 | ||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||
| OS: | Linux | ||||||||||||||||||
| Whiteboard: | |||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
Adding Toni who recently played with the Spawner class Adding James who recently played with the Spawner class And Pawel, who was also involved with that class recently. When using normal java.io.* it's not usual for waitFor() to close the streams. ctrl-T on Process and take a look at the implementation of UnixProcess.java.
I believe the usual pattern is:
Process p = null
try {
p = ...
int val = proc.waitFor();
} finally {
if (p != null) p.destroy();
}
#destroy should close the streams.
Also it's worth manually checking that the streams returned by Spawner don't mind being closed more than once. (We saw an issue in core.resources where an efs implementor didn't like that...)
We in PTP are seeing this problem also. We use the spawner to run jobs on the local eclipse client, and after many such jobs, all the file descriptors on the system are consumed. If stream is closed already would it throw an exception? Usually if you get a process that does not use stdin you can close it earlier to avoid fd leaks (In reply to comment #6) > If stream is closed already would it throw an exception? Usually if you get > a process that does not use stdin you can close it earlier to avoid fd leaks It looks like PTYOutputStream, for example, would throw an exception. Normal platform IOStreams allow you to close() them more than once without reporting an exception. (In reply to comment #7) > (In reply to comment #6) > > If stream is closed already would it throw an exception? Usually if you get > > a process that does not use stdin you can close it earlier to avoid fd leaks > > It looks like PTYOutputStream, for example, would throw an exception. Normal > platform IOStreams allow you to close() them more than once without reporting > an exception. Just to be safe should we replace try { getErrorStream().close(); getInputStream().close(); gettOutputStream().close(); } catch (IOException e) { } with try { getErrorStream().close(); } catch (IOException e) {} try { getInputStream().close(); } catch (IOException e) {} try { getOutputStream().close();} catch (IOException e) {} (In reply to comment #4) > When using normal java.io.* it's not usual for waitFor() to close the streams. > ctrl-T on Process and take a look at the implementation of UnixProcess.java. > > I believe the usual pattern is: > > Process p = null > try { > p = ... > int val = proc.waitFor(); > } finally { > if (p != null) p.destroy(); > } > > #destroy should close the streams. That looked weird to me too. I will change it. > Also it's worth manually checking that the streams returned by Spawner don't > mind being closed more than once. (We saw an issue in core.resources where an > efs implementor didn't like that...) I gather we're ok for this, based on the comments that followed, or did I mis-understand? Created attachment 195111 [details]
DSF-GDB part of the fix
This patch addresses the lack of cleanup in DSF-GDB, which was missing a couple of calls to destroy().
Committed to HEAD.
(In reply to comment #8) > try { getErrorStream().close(); } catch (IOException e) {} > try { getInputStream().close(); } catch (IOException e) {} > try { getOutputStream().close();} catch (IOException e) {} Looks fine to me. Created attachment 195112 [details]
Fix for Spawner
This patch is for Spawner. It removes the closing of the streams from waitFor() and assumes that destroy() should be called to cleanup. It also closes each of the three streams in its own try/catch, I noticed that UnixProcess does not do that, but I played it safe and made the change anyway.
This is not my area of expertise, so, being close to the final release, can someone give a +1 on this patch before I commit?
(In reply to comment #12) > This patch is for Spawner. It removes the closing of the streams from > waitFor() and assumes that destroy() should be called to cleanup. It also > closes each of the three streams in its own try/catch, I noticed that > UnixProcess does not do that, but I played it safe and made the change anyway. Sigh, I should have looked more closely. As Spawner#waitFor() does #close() I don't think we can change that at this stage (we'll break anyone else who doesn't correctly call #destroy). I think I would just make it look the same as your #destroy implementation. Otherwise looks ok to me. Looking at the history of Spawner I saw that there is some history in bug 102043. Mikhail was involved with that so I'm CCing him. The change from version 1.12 to 1.13 introduced the bad check for if (null == err) which seems to be copy/paste typo from the original patch. However, the change also removed the closing of the streams from waitFor(), like we are proposing here. However, a couple of months after that, going to version 1.14, the closing of streams was re-introduced in waitFor() by Mikhail. This makes be hesitant to remove it. So, unless Mikhail feels it is ok to remove the closing of the streams from waitFor(), I suggest we keep it. Created attachment 195118 [details] Better fix for Spawner (In reply to comment #13) > Sigh, I should have looked more closely. As Spawner#waitFor() does #close() I > don't think we can change that at this stage (we'll break anyone else who > doesn't correctly call #destroy). > > I think I would just make it look the same as your #destroy implementation. > Otherwise looks ok to me. We crossed :-) Thanks for finding the reason why we need to keep the closing of streams. I put things back as they were, and put a comment to explain. I committed this patch to HEAD. Thanks for the quick comments and review. Fixed *** cdt cvs genie on behalf of mkhouzam *** Bug 345164: Avoid leaking pipes [*] LaunchUtils.java 1.20 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/launching/LaunchUtils.java?root=Tools_Project&r1=1.19&r2=1.20 [*] GDBBackend.java 1.25 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/GDBBackend.java?root=Tools_Project&r1=1.24&r2=1.25 [*] Spawner.java 1.23 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.core/utils/org/eclipse/cdt/utils/spawner/Spawner.java?root=Tools_Project&r1=1.22&r2=1.23 (In reply to comment #14) > The change from version 1.12 to 1.13 introduced the bad check for > if (null == err) > which seems to be copy/paste typo from the original patch. I'm not sure this is a typo. The stream wrapper objects are only created on #get*Stream(), so this code is just closing any streams which the user didn't explicitly #get. i.e. it's expecting the user to be responsible for #close()ing any streams under her control. The issue could be that there's some code that attempts to #read after #waitFor. This might be possible in the previous implementation, but won't be possible if the streams are close()d, right? (In reply to comment #18) > (In reply to comment #14) > > The change from version 1.12 to 1.13 introduced the bad check for > > if (null == err) > > which seems to be copy/paste typo from the original patch. > > I'm not sure this is a typo. The stream wrapper objects are only created on > #get*Stream(), so this code is just closing any streams which the user didn't > explicitly #get. > i.e. it's expecting the user to be responsible for #close()ing any streams > under her control. That is an interesting possibility. I dug a little deeper and looking at version 1.12 we can see that all three streams used to always be created (in exec()) and were always closed() (in destroy() and waitFor()). At that time, there most probably was no leak. Going to version 1.13, the streams were only created once get*Stream() was called, which could (and did) leave some streams uncreated. It looks very probable to me, that the person making that change would want to add a check if (null != err) to avoid trying to close streams that were not created (which could not happen before). But they seem to have made a typo (copy/pasted from the new get*Stream() code which has if (null == err) ). It sounds less likely that we used to close all streams that were created, and then changed it to only close streams that were not created. > The issue could be that there's some code that attempts to #read after > #waitFor. This might be possible in the previous implementation, but won't be > possible if the streams are close()d, right? That is true. Except that the original implementation did close the streams, so this might have been a fluke. Just to confirm even more, I added the if (null == err) check (for in/out also) and ran CDI. In that case, I still see the leak of pipes. So, if it was actually: > i.e. it's expecting the user to be responsible for #close()ing any streams > under her control. then we wouldn't be doing it right in CDT itself. So, I'm leaning towards it being a typo. What do you think? (In reply to comment #19) > It looks very > probable to me, that the person making that change would want to add a check > if (null != err) > to avoid trying to close streams that were not created (which could not happen > before). But they seem to have made a typo (copy/pasted from the new > get*Stream() code which has if (null == err) ). It sounds less likely that we > used to close all streams that were created, and then changed it to only close > streams that were not created. I think this is incorrect. It's not the Java objects that are getting cleaned up, it's native OS resources (file descriptors in this case). It looks to me like the filedescriptors in 'fChannels' are created by a native call to #exec0 from run. They're destroyed by #close0() in the input stream implementations So my reading of the code is that it expects anyone who calls #get*Stream() to subsequently explicitly call #close() on the stream. waitFor and destroy() were only cleaning up filedescriptors that the calling code hasn't explicitly fetched. (In reply to comment #19) > Just to confirm even more, I added the > if (null == err) > check (for in/out also) and ran CDI. In that case, I still see the leak of > pipes. So, if it was actually: By all means CDI might have a bug... I definitely agree that #destroy should cleanup everything. I'm just worried about #waitFor doing so. (In reply to comment #20) > (In reply to comment #19) > > It looks very > > probable to me, that the person making that change would want to add a check > > if (null != err) > > to avoid trying to close streams that were not created (which could not happen > > before). But they seem to have made a typo (copy/pasted from the new > > get*Stream() code which has if (null == err) ). It sounds less likely that we > > used to close all streams that were created, and then changed it to only close > > streams that were not created. > > I think this is incorrect. It's not the Java objects that are getting cleaned > up, it's native OS resources (file descriptors in this case). It looks to me > like the filedescriptors in 'fChannels' are created by a native call to #exec0 > from run. Right. > They're destroyed by #close0() in the input stream implementations Where is this? I couldn't find it. > So my reading of the code is that it expects anyone who calls #get*Stream() to > subsequently explicitly call #close() on the stream. waitFor and destroy() > were only cleaning up filedescriptors that the calling code hasn't explicitly > fetched. I do see your point and you've got me on the fence :-) Let me ask one more question though. In version 1.12, all three streams would be closed in destroy() and waitFor(), no matter what. It could have been that there was no way of knowing if someone else was using the streams so we had to close them ourselves; it is hard to be sure. However, it was safe to close all three streams in both destroy() and waitFor() at that time. I guess you are thinking it may not be safe anymore (see below). > (In reply to comment #19) > > Just to confirm even more, I added the > > if (null == err) > > check (for in/out also) and ran CDI. In that case, I still see the leak of > > pipes. So, if it was actually: > > By all means CDI might have a bug... Agreed. DSF-GDB too then :-) > I definitely agree that #destroy should cleanup everything. I'm just worried > about #waitFor doing so. You are worried that we may close the streams before someone else is finished reading them? This was not "allowed" with version 1.12 or before, but I guess it could have happened since then. I'll add Mikhail on a review flag to see if he has any input on that. (In reply to comment #21) > > They're destroyed by #close0() in the input stream implementations > > Where is this? I couldn't find it. SpawnerInputStream and SpawnerOutputStream are both wrappers on a simple int fd. The native backend does the cleanup. > I do see your point and you've got me on the fence :-) > Let me ask one more question though. In version 1.12, all three streams would > be closed in destroy() and waitFor(), no matter what. It could have been that > there was no way of knowing if someone else was using the streams so we had to > close them ourselves; it is hard to be sure. However, it was safe to close all > three streams in both destroy() and waitFor() at that time. Agreed. > You are worried that we may close the streams before someone else is finished > reading them? This was not "allowed" with version 1.12 or before, but I guess > it could have happened since then. Indeed this seems confirmed by commit 1.13 in bug 102043 comment 7. It says that if close is called before the asynch console has read(), then bad things happen... Of course this will bug will occur too if destroy() closes streams being accessed concurrently. > > I definitely agree that #destroy should cleanup everything. I'm just worried > > about #waitFor doing so. > > You are worried that we may close the streams before someone else is finished > reading them? This was not "allowed" with version 1.12 or before, but I guess > it could have happened since then. It is very common that a process terminates before its output has been consumed, so I assume this fix reintroduces bug 102043. I think the closing of streams in waitFor() is a compromise between following the rule that the client is responsible for closing the streams and reducing the number of leaked file descriptors for clients which fail to close all streams. A better fix should be to implement finalize() on the Spawner streams and let the GC close eventually. This would be similar to the File(Input|Output)Streams used by the JDK Process implementations. (In reply to comment #23) > A better fix should be to implement finalize() on the Spawner streams and let > the GC close eventually. This would be similar to the > File(Input|Output)Streams used by the JDK Process implementations. +1. Of course this is no substitute for actually closing the streams explicitly. If Marc's seeing a leak, then there's some code somewhere that's not cleaning up correctly. In unit tests this will likely come back to bite, as GC can't reliably cleanup (non memory-related) OS resources. (In reply to comment #24) > Of course this is no substitute for actually closing the streams explicitly. If > Marc's seeing a leak, then there's some code somewhere that's not cleaning up > correctly. In unit tests this will likely come back to bite, as GC can't > reliably cleanup (non memory-related) OS resources. Agreed, it's better to close explicitly if possible. (In reply to comment #25) > (In reply to comment #24) > > Of course this is no substitute for actually closing the streams explicitly. If > > Marc's seeing a leak, then there's some code somewhere that's not cleaning up > > correctly. In unit tests this will likely come back to bite, as GC can't > > reliably cleanup (non memory-related) OS resources. > > Agreed, it's better to close explicitly if possible. So any outside class that calls Spawner.get*Stream() must close the streams explicitly? They can't rely on the destroy(). I'll make the following change: - revert Spawner to its previous version and add comments - close streams in DSF-GDB - close streams in CDI As for comment #5 from Greg, he will need to close the streams explicitly as well. (In reply to comment #26) > So any outside class that calls Spawner.get*Stream() must close the streams > explicitly? They can't rely on the destroy(). I think it is sensible to close _all_ streams on destroy(). That's also what the JDK impl does. On the other hand it could break client code, so it's a difficult decision. (In reply to comment #27) > (In reply to comment #26) > > So any outside class that calls Spawner.get*Stream() must close the streams > > explicitly? They can't rely on the destroy(). > > I think it is sensible to close _all_ streams on destroy(). That's also what > the JDK impl does. On the other hand it could break client code, so it's a > difficult decision. I guess we should play it safe and not risk breaking clients late in the cycle. I'll post a patch and see what you guys think, (In reply to comment #23) > A better fix should be to implement finalize() on the Spawner streams and let > the GC close eventually. This would be similar to the > File(Input|Output)Streams used by the JDK Process implementations. I'll add a finalize() to Spawner too. And lots of comments and javadoc ;-) (In reply to comment #23) > I think the closing of streams in waitFor() is a compromise between following > the rule that the client is responsible for closing the streams and reducing > the number of leaked file descriptors for clients which fail to close all > streams. I didn't get this. Doesn't destroy() do that anyway? Why do it in waitFor()? (In reply to comment #29) > > I think the closing of streams in waitFor() is a compromise between following > > the rule that the client is responsible for closing the streams and reducing > > the number of leaked file descriptors for clients which fail to close all > > streams. > > I didn't get this. Doesn't destroy() do that anyway? Why do it in waitFor()? I think Toni is referring to any places which don't #destroy(), but #close() streams. In this case #waitFor needs to cleanup non-requested streams: if(null == err) ((SpawnerInputStream)getErrorStream()).close(); (In reply to comment #29) > I didn't get this. Doesn't destroy() do that anyway? Why do it in waitFor()? You are not required to destroy() a process. A usual use case is the following: Runtime.getRuntime().exec(...).waitFor(); Doing this with the Spawner without closing the unused file descriptors, many file descriptors would be leaked. I assume that's the main reason for the current implementation of waitFor(). (In reply to comment #30) > I think Toni is referring to any places which don't #destroy(), but #close() > streams. In this case #waitFor needs to cleanup non-requested streams: > if(null == err) > ((SpawnerInputStream)getErrorStream()).close(); (In reply to comment #31) > You are not required to destroy() a process. A usual use case is the > following: > > Runtime.getRuntime().exec(...).waitFor(); > > Doing this with the Spawner without closing the unused file descriptors, many > file descriptors would be leaked. I assume that's the main reason for the > current implementation of waitFor(). Got it. Thanks both (In reply to comment #26) > As for comment #5 from Greg, he will need to close the streams explicitly as > well. Ok. I've modified my code to close the streams before calling #destroy(). I'll let you know if I see the problem again. Thanks! Created attachment 195225 [details]
Reverting Spawner
This patch reverts Spawner, adds comments, closes streams in their own try/catch, and adds the finalize().
The finalize() does not seem to work though and closing the stream does not release the pipe. I think it is because the native code has already marked the fileDescriptor as closed.
Created attachment 195226 [details]
Fix for DSF-GDB and CDI
This patch closes the streams explicitly in DSF-GDB and in CDI. With this patch and the one to reverting Spawner I don't get leaks anymore.
However, there may be other places in CDT where we dont' close the streams properly.
What do you think about this solution instead?
Created attachment 195249 [details]
Reverting Spawner without finalize()
Since the finalize() didn't seem to make any difference, I removed it. So, this patch reverts Spawner to version 1.22 (before this bug), but adds comments and individual try/catch.
I'm going to commit to HEAD, along with the DSF-GDB and CDI patch.
If you prefer to keep the finalize() or make any other changes, please let me know.
Thanks James and Toni for saving me from making a bad change! Marc, I don't think my review is required, James and Toni are much more competent. (In reply to comment #38) > Marc, I don't think my review is required, James and Toni are much more > competent. Sure, I think we're ok now. Thanks anyway. (In reply to comment #36) > If you prefer to keep the finalize() or make any other changes, please let me > know. The finalize would need to be on the SpawnerInputStream and SpawnerOutputStream as the clean up needs to be done when the streams are no longer reachable (which may or may not be the same as when the Spawner object stops being reachable). That said fixing the bug at the source is good. If the tests run with a large heap, then when the objects get finalized may be very far in the future from where the streams fall out of scope. *** cdt cvs genie on behalf of mkhouzam *** Bug 345164: Revert changes to Spawner and instead explicitly close streams in DSF-GDB and CDI [*] MIProcessAdapter.java 1.22 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.mi.core/src/org/eclipse/cdt/debug/mi/core/MIProcessAdapter.java?root=Tools_Project&r1=1.21&r2=1.22 [*] LaunchUtils.java 1.21 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/launching/LaunchUtils.java?root=Tools_Project&r1=1.20&r2=1.21 [*] GDBBackend.java 1.26 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/GDBBackend.java?root=Tools_Project&r1=1.25&r2=1.26 [*] Spawner.java 1.24 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.core/utils/org/eclipse/cdt/utils/spawner/Spawner.java?root=Tools_Project&r1=1.23&r2=1.24 (In reply to comment #40) > (In reply to comment #36) > > If you prefer to keep the finalize() or make any other changes, please let me > > know. > > The finalize would need to be on the SpawnerInputStream and SpawnerOutputStream > as the clean up needs to be done when the streams are no longer reachable > (which may or may not be the same as when the Spawner object stops being > reachable). Good point. I had missed that. But adding @Override protected void finalize() throws Throwable { close(); } to either one, does not release the pipe, even thought the finalize() does get called. I've stepped into close() and saw that fd == -1, which makes me think this happened in the native code. Any ideas? > That said fixing the bug at the source is good. If the tests run with a large > heap, then when the objects get finalized may be very far in the future from > where the streams fall out of scope. True. (In reply to comment #42) > Any ideas? Out of my depth, I'm afraid... Toni, to be safe, everything ok by you? (In reply to comment #44) > Toni, to be safe, everything ok by you? That's OK. I still think we should implement finalize in the stream classes and the Spawner itself to protect against clients which don't close streams or don't waitFor or destroy. I have committed a follow up fix for that. *** cdt cvs genie on behalf of aleherbau *** Bug 345164 - Spawner leaks pipes [*] SpawnerInputStream.java 1.18 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.core/utils/org/eclipse/cdt/utils/spawner/SpawnerInputStream.java?root=Tools_Project&r1=1.17&r2=1.18 [*] Spawner.java 1.25 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.core/utils/org/eclipse/cdt/utils/spawner/Spawner.java?root=Tools_Project&r1=1.24&r2=1.25 [*] SpawnerOutputStream.java 1.10 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.core/utils/org/eclipse/cdt/utils/spawner/SpawnerOutputStream.java?root=Tools_Project&r1=1.9&r2=1.10 [*] PTYOutputStream.java 1.11 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.core/utils/org/eclipse/cdt/utils/pty/PTYOutputStream.java?root=Tools_Project&r1=1.10&r2=1.11 [*] PTYInputStream.java 1.15 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.core/utils/org/eclipse/cdt/utils/pty/PTYInputStream.java?root=Tools_Project&r1=1.14&r2=1.15 (In reply to comment #45) > (In reply to comment #44) > > Toni, to be safe, everything ok by you? > > That's OK. I still think we should implement finalize in the stream classes and > the Spawner itself to protect against clients which don't close streams or > don't waitFor or destroy. I have committed a follow up fix for that. The fix looks good to me, however, I don't see it free the pipe (see comment #42). I tried like this, on Linux: 1- comment out GDBBackend line 448-450, to prevent it from closing the output stream. 2- set a breakpoint on SpawnerOutputStream.finalize 3- launch DSF-GDB then terminate it 4- remove the launch from the debug view 5- trigger GC using the Heap toolbar thingy 6- SpawnerOutputStream.finalize got hit, and I stepped into close() => fd == -1 in this case and nothing more gets done. So, I'm ok with the fix, but I wonder if it is useful. (In reply to comment #47) > So, I'm ok with the fix, but I wonder if it is useful. It is useful if you don't cleanup properly yourself. In the case you mention, there seems to be a leak of the Spawner object itself which is preventing the GC to clean up. That's why explicitly closing streams is a good idea. (In reply to comment #48) > (In reply to comment #47) > > So, I'm ok with the fix, but I wonder if it is useful. > > It is useful if you don't cleanup properly yourself. > In the case you mention, there seems to be a leak of the Spawner object itself > which is preventing the GC to clean up. That's why explicitly closing streams > is a good idea. Not sure I understand. I saw finalize() be called, so the GC worked. But the pipe didn't get removed. (In reply to comment #49) > Not sure I understand. I saw finalize() be called, so the GC worked. But the > pipe didn't get removed. I tried with some code I have here that uses Spawner, and the pipe was correctly cleaned up by the GC, having disabled my explicit call to close() + destroy(). I verified that the fd disappeared from the /proc having returned from close0(). If you're seeing fd == -1 (comment 47) when finalize is called then something else is closing it -- as that's the only way fd can become -1, right? (In reply to comment #50) > (In reply to comment #49) > > Not sure I understand. I saw finalize() be called, so the GC worked. But the > > pipe didn't get removed. > > I tried with some code I have here that uses Spawner, and the pipe was > correctly cleaned up by the GC, having disabled my explicit call to close() + > destroy(). I verified that the fd disappeared from the /proc having returned > from close0(). > > If you're seeing fd == -1 (comment 47) when finalize is called then something > else is closing it -- as that's the only way fd can become -1, right? Right, so I assumed it was the native library code. I'll try again and see if I see it. (In reply to comment #50) > (In reply to comment #49) > > Not sure I understand. I saw finalize() be called, so the GC worked. But the > > pipe didn't get removed. > > I tried with some code I have here that uses Spawner, and the pipe was > correctly cleaned up by the GC, having disabled my explicit call to close() + > destroy(). I verified that the fd disappeared from the /proc having returned > from close0(). > > If you're seeing fd == -1 (comment 47) when finalize is called then something > else is closing it -- as that's the only way fd can become -1, right? I still see fd == -1 and the leak of a pipe. More detailed steps: 1- comment out GDBBackend line 448-450, to prevent it from closing the output stream. 2- set a breakpoint on SpawnerOutputStream.finalize 2.1 on a shell do: lsof -p <pid of your eclipse> | grep pipe | wc -l this gives you the number of pipes used 3- launch DSF-GDB 3.1 lsof -p <pid of your eclipse> | grep pipe | wc -l you should see three more pipes than before, for the running GDB process 3.2 terminate the debug session 4- remove the launch from the debug view 4.1 lsof -p <pid of your eclipse> | grep pipe | wc -l you should see that only two pipes have been removed, the last one is the output stream that we are not closing in GDBBackend because of step 1 above. 5- trigger GC using the Heap toolbar thingy 6- SpawnerOutputStream.finalize gets hit, step into close() => fd == -1 in this case and nothing more gets done. 7- lsof -p <pid of your eclipse> | grep pipe | wc -l still shows an extra pipe leaked. (In reply to comment #52) > I still see fd == -1 and the leak of a pipe. More detailed steps: > > 1- comment out GDBBackend line 448-450, to prevent it from closing the output > stream. > 2- set a breakpoint on SpawnerOutputStream.finalize > 2.1 on a shell do: > lsof -p <pid of your eclipse> | grep pipe | wc -l > this gives you the number of pipes used > > 3- launch DSF-GDB > 3.1 lsof -p <pid of your eclipse> | grep pipe | wc -l > you should see three more pipes than before, for the running GDB process > > 3.2 terminate the debug session > 4- remove the launch from the debug view > 4.1 lsof -p <pid of your eclipse> | grep pipe | wc -l > you should see that only two pipes have been removed, the last one is the > output stream that we are not closing in GDBBackend because of step 1 above. > > 5- trigger GC using the Heap toolbar thingy > 6- SpawnerOutputStream.finalize gets hit, step into close() > => fd == -1 in this case and nothing more gets done. > > 7- lsof -p <pid of your eclipse> | grep pipe | wc -l > still shows an extra pipe leaked. I did try this example and found exactly the same - finalize never closes any open stream. The only explanation is that the open SpawnerOutputStream is still alive - which led me to investigate the memory leaks surrounding Spawner and DsfSession. Above steps with current HEAD would still leak the Spawner and SpawnerOutputStream, AFAIK. Did you verify that no instance of SpawnerOutputStream was still alive? (In reply to comment #53) > I did try this example and found exactly the same - finalize never closes any > open stream. The only explanation is that the open SpawnerOutputStream is still > alive - which led me to investigate the memory leaks surrounding Spawner and > DsfSession. Above steps with current HEAD would still leak the Spawner and > SpawnerOutputStream, AFAIK. > Did you verify that no instance of SpawnerOutputStream was still alive? I hadn't verified that, but now that I did, I see it is because of the DefaultVMModelProxyStrategy leak (bug 345476). Once I kill the Debug view, the pipes are released. So the finalize() that was seeing being hit was for the other streams (I should have payed attention to that). So, the finalize() solution does work and will be useful, once we fix bug 345476. Thanks for your patience Toni. |
Created attachment 195095 [details] Proposed fix When running many launches through JUnit tests, I was getting a leak which stopped the tests after about 150 launches. It turned that that it was not a memory leak but a leak of pipes. Using the linux command 'lsof' and looking at how many pipes were open, I noticed that a DSF-GDB launch was leaking 5 pipes, while a CDI was leaking 2 pipes. The difference is due to the fact that DSF-GDB launches GDB twice (one to get the version with gdb --version, which wasn't being cleaned-up). This lead me to CDT's Spawner class. First, the code that cleans-up the streams, seems wrong to me: public synchronized void destroy() { if(null == err) getErrorStream().close(); if(null == in) getInputStream().close(); if(null == out) getOutputStream().close(); all these checks should be looking for != null, or else we only close streams that we didn't open. However, after fixing this, I still had a leak, on the streams that we hadn't opened. It seems that when spawning a process, all three pipes (in/out/err) are opened by the call to exec0 (part of exec_unix.c). And, even if we don't make use of all of them (we don't use 'err'), that pipe needs to be connected and then closed, if we want to release the pipe. So, the attached patch, when cleaning up, calls get???Stream().close() all the time. If the stream was created, it will be closed; if it was not created, it will be created by the call to get???Stream() which will connect to the pipe, and then we will close it, and release the pipe. This is all on Linux, and I'm not sure of any impacts on Windows. But since the current code, actually calls get???Stream() for the stream we didn't open, the change should be safe. With this patch, I can run 450 launches before actually running of memory.