Hi Jacques, What you are closing is socket, which is the lowest abstraction in the chained streams. Java automatically closes from the highest streams chained through constructors, not lowest, and hence leaving streams in memory, This is a very common source of memory leaks in Java and a basic thing to care for when working with closable streams. It is so popular that to reduce code verbosity in Java 7 they introduced AutoClosable as a parent to Closable to be used with try-with-resources blocks to ease the work.
Also, the code is now a jumbled code, you're closing twice upon an exception, this does not have an effect but is a sign of problematic code. The whole code block can be improved substantially. That is why I am suggesting a rewrite after a thorough review. On Sat, Sep 3, 2016 at 12:31 PM, Jacques Le Roux < [email protected]> wrote: > Hi Taher, Jacopo, > > Sincerely reading https://docs.oracle.com/javase > /8/docs/api/java/net/Socket.html#close-- > > /<<public void close() > throws IOException > Closes this socket. > Any thread currently blocked in an I/O operation upon this socket will > throw a SocketException. > Once a socket has been closed, it is not available for further networking > use (i.e. can't be reconnected or rebound). A new socket needs to be > created. > Closing this socket will also close the socket's InputStream and > OutputStream. > If this socket has an associated channel then the channel is closed as > well.>> > / > > the only problem I can see is if several threads would simultaneously > access the same PcChargeApi object and its inner Socket has been closed > then a SocketException will be fired./ > / > > But in the PcChargeServices class, each call to PcChargeApi.send() is done > after the creation of a local PcChargeApi object through > PcChargeApi.getApi(). So I can't see how this object could be shared. > > Hence closing the socket, as I did in my last commit, should not "leads to > possible memory leaks". By closing it I close also the socket's InputStream > so no leaks should be expected. > > BTW you may wonder (I did!), because PcChargeApi.getApi() returns an > escaped PcChargeApi object. So it's not thread safe, and another thread > could access it concurrently (being a PcChargeServices.ccAuth() or any > other of the PcChargeServices.cc*() public methods) > > And if several threads simultaneously access the same PcChargeApi object > and its Socket has been closed, then a SocketException would be fired. > > But, because the Socket does not escape from the PcChargeApi.send(), the > Socket is thread safe and can't be shared between threads (even if the > PcChargeApi object can be) and I don't see how a SocketException could be > fired/// > / > > So I see no problem with my currently committed solution. I initially > missed to close the Socket in the catched exceptions which is why, I > believe, Jacopo was concerned. > > If I miss something please explain > > Note that in the original code the socket was not closed, so a resource > leak existed. > It's not a big deal if you don't use a lot of calls to PcChargeApi.send() > during the JVM session, since the JVM will clear it when finished. > But it seems to me that it's better to avoid crossing this issue. > > And last but not least, this is in a peripheral thirdparty\gosoftware, so > there is currently nothing to urgently worry about in trunk. We can > exchange and I'd be happy to stand corrected if I get a clear answer. > > Jacques > > > > Le 02/09/2016 à 20:43, Taher Alkhateeb a écrit : > >> Hi Jacques, >> >> The resources are not properly closed in your commit. It would take time >> to >> explain, suffice to say that your code leads to possible memory leaks. >> That >> is why I suggest a full review and carefully applying the stream memory >> management. There are multiple ways to fix this, some related to Java 7, >> but it would take time to explain, and it is your initiative so I leave it >> up to you to fix it. >> >> Regards, >> >> Taher Alkhateeb >> >> On Fri, Sep 2, 2016 at 8:37 PM, Jacques Le Roux < >> [email protected]> wrote: >> >> Why should I not close the Socket once the DataInputStream derived from it >>> has been used? >>> >>> In the case of an exception occurs will not the resource be leaked? >>> >>> Do you mean that I should also close the DataInputStream before closing >>> the Socket? >>> >>> ie something like >>> >>> Index: applications/accounting/src/main/java/org/apache/ofbiz/accou >>> nting/thirdparty/gosoftware/PcChargeApi.java >>> =================================================================== >>> --- applications/accounting/src/main/java/org/apache/ofbiz/accou >>> nting/thirdparty/gosoftware/PcChargeApi.java (revision 1758990) >>> +++ applications/accounting/src/main/java/org/apache/ofbiz/accou >>> nting/thirdparty/gosoftware/PcChargeApi.java (working copy) >>> @@ -200,14 +200,17 @@ >>> try { >>> outDoc = UtilXml.readXmlDocument(buf.toString(), >>> false); >>> } catch (ParserConfigurationException e) { >>> + dis.close(); >>> sock.close(); >>> throw new GeneralException(e); >>> } catch (SAXException e) { >>> + dis.close(); >>> sock.close(); >>> throw new GeneralException(e); >>> } >>> >>> PcChargeApi out = new PcChargeApi(outDoc); >>> + dis.close(); >>> sock.close(); >>> return out; >>> } else { >>> >>> Jacques >>> >>> >>> >>> Le 02/09/2016 à 19:16, Taher Alkhateeb a écrit : >>> >>> Jacques this is still problematic, you are closing the streams upon >>>> exceptions! now all kinds of memory leaks and side effects could take >>>> place >>>> because you are not ensuring closing all streams properly. >>>> >>>> This needs a revert and a full review to make things tight in terms of >>>> memory. >>>> >>>> On Fri, Sep 2, 2016 at 8:08 PM, Jacques Le Roux < >>>> [email protected]> wrote: >>>> >>>> Le 02/09/2016 à 18:46, Jacopo Cappellato a écrit : >>>> >>>>> On Fri, Sep 2, 2016 at 3:19 PM, Jacques Le Roux < >>>>> >>>>>> [email protected]> wrote: >>>>>> >>>>>> Le 02/09/2016 à 12:30, Jacopo Cappellato a écrit : >>>>>> >>>>>> ... >>>>>>> 2) I suspect that you are closing the socket connection too early >>>>>>> >>>>>>> in PcChargeApi >>>>>>> >>>>>>>> I see no problem with this point, the "sock" socket connection is >>>>>>>> not >>>>>>>> >>>>>>>> used >>>>>>> in code below. >>>>>>> ... >>>>>>> Jacques >>>>>>> >>>>>>> >>>>>>> Jacques, >>>>>>> >>>>>>> at the moment I don't have time to explain but after your response >>>>>> above I >>>>>> am a bit concerned about the effort that you are carrying on, because >>>>>> you >>>>>> may break other code without even suspecting it. >>>>>> I have to go now, if others can explain then great, otherwise I will >>>>>> follow >>>>>> up with you asap. >>>>>> >>>>>> Jacopo >>>>>> >>>>>> Ah I think I got your point when back, I should better close the >>>>>> Socket >>>>>> >>>>>> later, done at r1758990 >>>>> >>>>> Jacques >>>>> >>>>> >>>>> >>>>> >
