[jcifs] Hung thread in SmbTree.java

[Michael B Allen]

On Wed, Feb 18, 2015 at 2:52 PM, Rich Hammond <rich.hammond at vmturbo.com> wrote:
> 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)
<snip>
>
>
> Looking at the sources for SmbTree I have a couple questions:
>
> 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.

> 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/