[jcifs] Hung thread in SmbTree.java

[Rich Hammond]

I tried to search the archives and I do not see this issue. If there is another place I should search, please let me know.
I have a thread that has been hung for days at:

java.lang.Thread.State: WAITING (on object monitor)

at java.lang.Object.wait(Native Method)

- waiting on <0x000000074edf9e60> (a jcifs.smb.SmbTransport)

at java.lang.Object.wait(Object.java:503)

at jcifs.smb.SmbTree.treeConnect(SmbTree.java:143)

- locked <0x000000074edf9e60> (a jcifs.smb.SmbTransport)

at jcifs.smb.SmbFile.doConnect(SmbFile.java:911)

at jcifs.smb.SmbFile.connect(SmbFile.java:954)

at jcifs.smb.SmbFile.connect0(SmbFile.java:880)

at jcifs.smb.SmbFileInputStream.<init>(SmbFileInputStream.java:76)

at jcifs.smb.SmbFileInputStream.<init>(SmbFileInputStream.java:65)

at jcifs.smb.SmbFile.getInputStream(SmbFile.java:2844)

at rpc.ncacn_np.RpcTransport.attach(RpcTransport.java:91)

at rpc.Stub.attach(Stub.java:106)

at rpc.Stub.call(Stub.java:110)

at org.jinterop.winreg.smb.JIWinRegStub.winreg_OpenHKCR(JIWinRegStub.java:132)

at org.jinterop.dcom.core.JIComServer.initialise(JIComServer.java:509)

at org.jinterop.dcom.core.JIComServer.<init>(JIComServer.java:414)

Looking at the sources for SmbTree I have a couple questions:

1. In the treeConnect method why does the catch bother to call treeDisconnect? The connectionState = 1 and treeDisconnect returns immediately in that case, so why bother calling at all?
2. Should not the rule be that anyplace that one sets connectionState = 0; you also call session.transport.notifyAll()? Why doesn't the catch block have a notifyAll()?
3. Alternatively, treeDisconnect() could be: void treeDisconnect( boolean inError ) {

synchronized (session.transport()) {

if (connectionState == 2) {
connectionState = 3; // disconnecting

if (!inError && tid != 0) {
try {
send( new SmbComTreeDisconnect(), null );
} catch( SmbException se ) {
if (session.transport.log.level > 1) {
se.printStackTrace( session.transport.log );
}
}
}

}
inDfs = false;
inDomainDfs = false;

connectionState = 0;

session.transport.notifyAll();
}
}


This message and its attachments are intended only for the designated recipient(s). It may contain confidential or proprietary information and may be subject to legal or other confidentiality protections. If you are not a designated recipient, you may not review, copy or distribute this message. If you receive this in error, please notify the sender by reply e-mail and delete this message. Thank you.

[Michael B Allen]

Post by Rich Hammond
I tried to search the archives and I do not see this issue. If there is
another place I should search, please let me know.
java.lang.Thread.State: WAITING (on object monitor)
at java.lang.Object.wait(Native Method)
- waiting on <0x000000074edf9e60> (a jcifs.smb.SmbTransport)
at java.lang.Object.wait(Object.java:503)
at jcifs.smb.SmbTree.treeConnect(SmbTree.java:143)
- locked <0x000000074edf9e60> (a jcifs.smb.SmbTransport)
at jcifs.smb.SmbFile.doConnect(SmbFile.java:911)

<snip>

Post by Rich Hammond
In the treeConnect method why does the catch bother to call treeDisconnect?
The connectionState = 1 and treeDisconnect returns immediately in that case,
so why bother calling at all?

Hi Rich,

No. It is possible for connectionState to change after set to 1
because something could call a method that calls wait() on the
transport which would unlock it. And in fact, session.send() calls
transport.send() which ultimately calls wait() on the transport.

Post by Rich Hammond
Should not the rule be that anyplace that one sets connectionState = 0; you
also call session.transport.notifyAll()? Why doesn’t the catch block have a
notifyAll()?

Yes. It looks like there is a flaw here. I have added this issue to
the TODO list.

But in this case I might try to just set connectionState = 0 in the
treeDisconnect where it returns and thus dodges the notifyAll and then
instead let it fall through to the existing notifyAll():

void treeDisconnect( boolean inError ) {
synchronized (session.transport()) {

if (connectionState != 2) { // not-connected
if (inError) {
connectionState = 0;
}
} else {
...
}

session.transport.notifyAll();
}
}

So the philosophy is that whenever connectionState changes to
anything, you always call notifyAll. There should not have been a
return in this block.

But this sort of code is non-trivial. I would have to play with it and
test it to make sure it's good so this is not the official fix.

Again, I put this on the TODO list for further investigation.

Thanks a lot for the feedback. Nice catch!

Mike

--
Java Active Directory Integration
http://www.ioplex.com/