Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/679
@ivmaykov I did another cycle of review the unit tests, sorry I still not
see value in denial-of-service tests, but maybe I don't see something
important. Let's put it this way:
Use case of `UnifiedServerSocket` is the following:
> Extend standard Socket class to catch all read events in order to trigger
TLS/Plaintext mode detection in a lazy fashion (catching the last chance of
doing so).
In order to do that it captures calls to the underlying input/output
streams and initiates detection of channel type on the first read/write
operation. So far so good.
The important bit here is that `UnifiedServerSocket` **doesn't contain
anything related to threading**. As a consequence if we write purely unit tests
for this class we won't have to verify anything which is related to threading.
DOS test comment says:
> This test makes sure that UnifiedServerSocket used properly (a single
thread accept()-ing connections and handing the resulting sockets to other
threads for processing) is not vulnerable to a simple denial-of-service attack
in which a client connects and never writes any bytes. **This should not block
the accepting thread, since the read to determine if the client is sending a
TLS handshake or not happens in the processing thread.**
And here's the thing: this test is testing the proper implementation of the
server (e.g. dealing with threads in the right way), which is implemented in
the test itself: `UnifiedServerThread`.
My 2 cents here is that if you think you haven't covered a user scenario or
an edge with the existing tests, you need to rewrite these tests to be more
strict and test that-and-only-that particular case which is missing.
I think the coverage is acceptable (what you mentioned in your latest
comment has already been covered), but there's no harm in adding more. I just
don't want tests which don't add value **or** adds value in an unreasonably
cumbersome way.
Please correct me if I'm wrong.
---
