> On July 5, 2016, 9:22 a.m., Jonathan Hurley wrote: > > I don't see how this actually fixes the problem; importing gc from > > subprocess would be the same as importing it normally, no? There isn't a > > different gc module that subprocess uses. Also, after you patch it, you > > then re-import it - wouldn't that break the existing patching? > > Andrew Onischuk wrote: > "I don't see how this actually fixes the problem" - this was tested on > the instance where I could reproduce this and is a sure fix. > > Jonathan Hurley wrote: > So, let's say that it does fix the issue by always reporting that GC is > enabled. Wouldn't this then cause side effects since Popen is specifically > disabling it for a reason? > > Andrew Onischuk wrote: > popen disables gc anyway no matter if it is enabled or not. > > Andrew Onischuk wrote: > And even with fix it still disables it. The fix the for the race > condition, when it doesn't re-enable it again after the work is done.
OK, so `subprocess` doesn't check before it disables it; I can see that. However, we're changing the behavior of a function provided by the gc and we don't know what kinds of issues this could cause on other distributions of Python. There could be other versions which check this function and/or turning on the gc when it's already on could cause a problem. - Jonathan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49535/#review140761 ----------------------------------------------------------- On July 4, 2016, 4:45 a.m., Andrew Onischuk wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49535/ > ----------------------------------------------------------- > > (Updated July 4, 2016, 4:45 a.m.) > > > Review request for Ambari and Dmitro Lisnichenko. > > > Bugs: AMBARI-17539 > https://issues.apache.org/jira/browse/AMBARI-17539 > > > Repository: ambari > > > Description > ------- > > *Reason of memory leak:* > Race condition in subprocess python module. > Due to this race condition at some unlucky cases python garbage collection > was disabled. > This usually happened when running alerts, as a bunch of our alerts run shell > commands and they do it in a different threads. > > *Fix for the issue:* > Synchronizing subprocess is not the best option. Since some people can still > use it without synchronization not knowing about the issue. > Also synchronizing will provide some unnecessary slowdown. So for this issue > the proposed fix is to monkey patch subprocess.gc.isenabled. > > > Diffs > ----- > > ambari-agent/src/main/python/ambari_agent/main.py 4db89f8 > > Diff: https://reviews.apache.org/r/49535/diff/ > > > Testing > ------- > > mvn clean test > > > Thanks, > > Andrew Onischuk > >
