OF-2559: Use Netty for LocalIncomingServerSessionTest #2232
Conversation
There was a problem hiding this comment.
Changes make sense: tests not passing without other changes mentioned so should probably wait until they have also been merged.
Can we delete SocketAcceptThread, BlockingAcceptingMode, SocketAcceptingMode, and ServerSocketReader now? It looks like these are all no longer used.
Some elements of the former implementation (SocketConnection etc) are still used for ServerDialback and other features of Openfire (perhaps these can be ticketed in OF Jira?)
xmppserver/src/test/java/org/jivesoftware/openfire/session/LocalIncomingServerSessionTest.java
Outdated
Show resolved
Hide resolved
xmppserver/src/test/java/org/jivesoftware/openfire/session/LocalIncomingServerSessionTest.java
Show resolved
Hide resolved
guusdk
force-pushed
the
OF-2559_Use-Netty-for-S2S-unittest
branch
from
8fcee2e
to
7e0c1d4
Compare
|
I very much want the old networking code to be removed, but didn't want to do it as part of this PR. As per @AlexGidman's suggestion, I have raised a new JIRA ticket for the removal effort: https://igniterealtime.atlassian.net/browse/OF-2635 The review suggestions were applied, and this PR has been rebased on the latest main. |
guusdk
force-pushed
the
OF-2559_Use-Netty-for-S2S-unittest
branch
from
fbdd25c
to
6065103
Compare
|
My optimizations seem to have improved the LocalIncomingServerSessionTest (unexpectedly), but have also made the LocalOutgoingServerSessionTest fail its 13'th test - but only if it is run as part of multiple tests. I'm guessing that I've somehow made tests interdependant (which is bad). |
Prior to this commit, the unit test for LocalIncomingServerSession used the old, pre-Netty code. With this commit, the Netty code is being used instead. Insteead of the old blocking code, the test now uses a (Netty-backed) ConnectionListener directly. To identify the LocalIncomingServderSession instance that is to be created, a streamID value is used, which is collected by the dummy server peer implementation for that purpose.
By calculating a 'valid' identity store (with a 'valid' certificate), each test no longer needs to recompute these instances. Profiling indicates that this saves roughly 66% of the CPU time spent in the LocalIncomingServerSessionTest
By re-using a ByteBuffer, instead of re-allocating a new byte[] for each read, the amount of memory allocations is changes from thousands of gigabytes to hundreds of megabytes.
As the unit test is being executed on a local network connection, we can reduce the session initialise timeout considerably. This makes tests that depend on that timeout execute in 20% of the time, which is a considerable runtime improvement.
guusdk
force-pushed
the
OF-2559_Use-Netty-for-S2S-unittest
branch
from
6065103
to
5486250
Compare
This prevents stack overflow when the getters use the toString implementation (for example, for debug logging).
guusdk
force-pushed
the
OF-2559_Use-Netty-for-S2S-unittest
branch
from
4ede455
to
eaa329c
Compare
Make sure to set 'authenticated' only after the internal state of 'session' itself is updated, to avoid race conditions.
|
Ah, I suspect that the new tests were failing off and on again from a race condition, not because session state was carried over. The race condition is fixed in commit 8f2f4f2 |
guusdk
force-pushed
the
OF-2559_Use-Netty-for-S2S-unittest
branch
from
15c4a76
to
e4289f2
Compare
|
This PR is running into issues that are likely addressed in another PR: #2226. That PR is running into issues fixed by this PR. To resolve this chicken/egg problem, I'm going to merge this PR, and rebase the other PR. |
Prior to this commit, the unit test for LocalIncomingServerSession used the old, pre-Netty code. With this commit, the Netty code is being used instead.
Insteead of the old blocking code, the test now uses a (Netty-backed) ConnectionListener directly. To identify the LocalIncomingServderSession instance that is to be created, a streamID value is used, which is collected by the dummy server peer implementation for that purpose.