Hi Jacques, I don't see anything wrong with Oracle's documentation. It is very much correct that closing a socket closes its streams. The problem is with closing the higher chained streams.
Taher Alkhateeb On Sep 3, 2016 3:16 PM, "Jacques Le Roux" <[email protected]> wrote: > Thanks Taher, > > When I read your answer I thought that then the Oracle JavaDoc below is a > least misleading. I mean specifically > > <<Closing this socket will also close the socket's InputStream > andOutputStream.>> > > They should have spoken about possible memory leaks :/ > > But it seems this area is a bit touchy (just Google for "java does closing > socket close streams"!) and I understand your point > > Actually I should have used my own advice (to use the try-with-resources > statement) from start on this case also. > > That's done at revision: 1759082 . I'll review the other changes I did > then to check I have not missed a similar case. > > Jacques > > > Le 03/09/2016 à 12:20, Taher Alkhateeb a écrit : > >> 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 >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >
