Github user ivmaykov commented on the issue:
https://github.com/apache/zookeeper/pull/679
@anmolnar I understand your point about the DoS tests, and agree that they
are not very focused and do end up testing the structure of the test code
itself in (i.e. that the implementation of
`UnifiedServerSocketTest$UnifiedServerThread` plays nicely with
`UnifiedServerSocket`). Let me explain really briefly how that test came about,
since the history behind it is missing in this discussion.
Initially when I rewrote `UnifiedServerSocket` and added the
`UnifiedSocket` inner class, I didn't have the `UnifiedInputStream` and
`UnifiedOutputStream` inner classes. I was careful to avoid any reads inside
`accept()`, and thought that was good enough. However, with this version of the
code we still saw problems on our test cluster during a stress
disconnect/reconnect test. Investigation showed evidence that the Leader's
`accept()` thread was getting stuck, and then I found the code in
`Leader$LearnerCnxAcceptor#run()` that calls `getInputStream()` on the accepted
socket and wraps the result in a `BufferedInputStream`. The only way at the
time to get the input stream was to resolve the type of socket (by doing a read
of first 5 bytes and then TLS/plaintext mode detection), and get the real
socket's input stream.
My fix involved adding the `UnifiedInputStream` and `UnifiedOutputStream`
inner classes, which allowed me to delay the mode detection. Now, instead of
triggering mode detection at the time of `getInputStream()`, we can trigger it
the first time the stream is used for I/O. I wrote the DoS test as part of the
same diff, and was careful to replicate the threading behavior of
`Leader$LearnerCnxAcceptor.run()` in `UnifiedServerThread`. I also verified
that the new tests failed on a version of the code that didn't have the unified
stream wrappers.
If I remove the DoS tests, the behavior of delaying the mode detection's
read to the point of first I/O operation would not be tested properly. I think
that code should be tested! However, if you prefer I could probably test it in
a more focused way (i.e. just test each method that's meant to be non-blocking
by itself). Would that be an acceptable compromise?
---
