zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ivmaykov <...@git.apache.org>
Subject [GitHub] zookeeper issue #679: ZOOKEEPER-3172: Quorum TLS - fix port unification to a...
Date Fri, 23 Nov 2018 18:29:49 GMT
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?


---

Mime
View raw message