All TcpTransports sharing one SessionManager leads to unintended Session cancellation

[thomasklunker]

Hi everyone, I have run into a problem recently.

In commit 9741741 from May 25, 2022, the Server class behavior was changed, so that all TcpTransport instances it creates share one SessionManager instance owned by the Server. Focus here on line 632:

https://github.com/locka99/opcua/blob/a5f452a67a0efbb1faaacda023612eea6d63be22/lib/src/server/server.rs#L626-L634

Now, once a TcpTransport finishes, meaning when an OPC-UA client disconnects, it calls TcpTransport::finish:

https://github.com/locka99/opcua/blob/a5f452a67a0efbb1faaacda023612eea6d63be22/lib/src/server/comms/tcp_transport.rs#L311

This method in turn calls SessionManager::clear on its SessionManager field, which, as I mentioned, is shared between all TcpTransports (client connections).

https://github.com/locka99/opcua/blob/a5f452a67a0efbb1faaacda023612eea6d63be22/lib/src/server/comms/tcp_transport.rs#L139-L140

SessionManager::clear terminates all Sessions, meaning all Sessions from other TcpTransports as well:

https://github.com/locka99/opcua/blob/a5f452a67a0efbb1faaacda023612eea6d63be22/lib/src/server/session.rs#L81-L90

This was not a problem before commit 9741741, when each TcpTransport had its own separate SessionManager, so it would only close its own Sessions. The way it is now, however, leads to the bug that I came upon and which is the reason I investigated this in the first place.

1. Start any (demo) OPC-UA server.
2. Connect 2 or more clients to that server.
3. Disconnect 1 client.
4. Try continue using any of the other clients. This fails with BadSessionIdInvalid since all sessions were closed.

I strongly suspect that this behavior is not intended. I managed to fix this issue by simply reversing the line change made in commit 9741741 that clones the SessionManager Arc into the TcpTransport, instead creating a new SessionManager for each TcpTransport. Now, from the commit message of 9741741 I can see that having a shared SessionManager is intentional, so this might not be the ideal solution to the problem. Maybe it would be preferrable to categorize the Sessions by TcpTransport in the SessionManager so that they can be closed separately.

Please tell me if I missed anything.

[kevincolyar]

I wonder if this is the same issue I'm having with #281

[thomasklunker]

I wonder if this is the same issue I'm having with #281

I can definitely see them being related.

[evanjs]

Pretty sure I'm encountering this lately Feels like it could be a priority issue

Per the current documented limitations:

Currently the following are not supported

• Multiple created sessions in a single transport.

If I understand the issue I/OP are experiencing, it almost seems like this "currently unsupported" scenario is (inadvertantly?) the way that the library currently functions, leading to the previously mentioned unintended session cancellation.