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
