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
