kirklund commented on a change in pull request #6063:
URL: https://github.com/apache/geode/pull/6063#discussion_r585945453
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java
##########
@@ -1550,12 +1574,64 @@ public void refuseHandshake(OutputStream out, String
message, byte exception) th
out.flush();
}
- private boolean handOffQueueInitialization(Socket socket, CommunicationMode
communicationMode) {
+ @Override
+ public void refuseHandshake(OutputStream out, String message, byte exception,
+ NioSslEngine sslEngine, Socket socket) throws IOException {
+ if (sslEngine == null) {
+ refuseHandshake(out, message, exception);
+ return;
+ }
+ try (ByteBufferOutputStream bbos =
+ new
ByteBufferOutputStream(sslEngine.getEngine().getSession().getPacketBufferSize()))
{
+
+ DataOutputStream dos = new DataOutputStream(bbos);
+
+ // Write refused reply
+ dos.writeByte(exception);
+
+ // write dummy endpointType
+ dos.writeByte(0);
+ // write dummy queueSize
+ dos.writeInt(0);
+
+ // Write the server's member
+ DistributedMember member =
InternalDistributedSystem.getAnyInstance().getDistributedMember();
Review comment:
Here's another call to reach out for a dependency and it's even worse
because it has to go to a singleton which we've been working to get rid of.
This one might be an example of one that should be passed into the constructor
since it never changes during the life of this acceptor instance:
```
private final DistributedMember member;
AcceptorImpl(..., DistributedMember member) {
...
this.member = member;
}
```
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java
##########
@@ -1550,12 +1574,64 @@ public void refuseHandshake(OutputStream out, String
message, byte exception) th
out.flush();
}
- private boolean handOffQueueInitialization(Socket socket, CommunicationMode
communicationMode) {
+ @Override
+ public void refuseHandshake(OutputStream out, String message, byte exception,
+ NioSslEngine sslEngine, Socket socket) throws IOException {
+ if (sslEngine == null) {
+ refuseHandshake(out, message, exception);
+ return;
+ }
+ try (ByteBufferOutputStream bbos =
+ new
ByteBufferOutputStream(sslEngine.getEngine().getSession().getPacketBufferSize()))
{
Review comment:
Here's another example of breaking the [Law of
Demeter](https://en.wikipedia.org/wiki/Law_of_Demeter) and exposing internal
knowledge that the acceptor shouldn't have to know about. It's better to
embrace dependency injection and just pass in `int packetBufferSize` or even
`javax.net.ssl.SSLSession sslSession` as a parameter.
One direction I think we really need to steer the code base towards is
[dependency
injection](https://en.wikipedia.org/wiki/Dependency_inversion_principle). The
idea is to pass in all dependencies, either to the constructor or as parameters
to a method call, instead of reaching out to fetch them.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java
##########
@@ -499,18 +508,29 @@
LinkedBlockingQueue<ByteBuffer> tmp_commQ = null;
Set<ServerConnection> tmp_hs = null;
SystemTimer tmp_timer = null;
+ BufferPool tmp_buffpool = null;
if (isSelector()) {
tmp_s = Selector.open(); // no longer catch ex to fix bug 36907
tmp_q = new LinkedBlockingQueue<>();
tmp_commQ = new LinkedBlockingQueue<>();
tmp_hs = new HashSet<>(512);
tmp_timer = new SystemTimer(internalCache.getDistributedSystem());
+ DistributionImpl distribution = (DistributionImpl) internalCache
Review comment:
Instead of importing and casting to `DistributionImpl` how about we
adjust the interface to provide everything the acceptor needs. We don't want to
drag in _DirectChannel_ and _Conduit_ either.
So if it makes sense to fetch a `BufferPool` from a `Distribution`, then I
would add this to the interface:
```
/**
* ...say something here about what the BufferPool is from the point of view
of callers that depend on Distribution...
*/
BufferPool getBufferPool();
```
Then the impl in `DistributionImpl` would be something like:
```
public BufferPool getBufferPool() {
return directChannel.getConduit().getBufferPool();
}
```
This prevents callers from having to know that a Distribution isa
DistributionImpl that hasa DirectChannel that hasa Conduit that hasa BufferPool
which breaks [SOLID principles](https://en.wikipedia.org/wiki/SOLID) (you can
google for better Java examples than wikipedia).
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/ServerConnection.java
##########
@@ -254,6 +254,8 @@ public static void emptyCommBufferPool() {
@MutableForTesting
private static boolean TEST_VERSION_AFTER_HANDSHAKE_FLAG;
+ private NioSslEngine sslEngine;
Review comment:
We need to strive to make fields like this `final`. There are several
places in the new code where you close the `NioSslEngine` and set the field
back to null. There are also lots of null checks. This makes the code thread
unsafe because there's no form of synchronization or use of volatile. Non-final
fields tend to lower performance and make the code a lot busier with null
checks.
Why do you really need this to be mutable? And what is the state change for
that changes this from non-null to null? Why does the `ServerConnection` remain
valid and usable after closing and nulling out the `NioSslEngine`?
There are two possible approaches I would take:
1) I would try very hard to make `NioSslEngine sslEngine` a final field.
I might even change it to be an interface so that I can either pass in a
`NioSslEngine` or a dummy implementation of the interface to the constructor. A
dummy implementation does nothing or returns values from methods that causes
the calling code to not use nio ssl.
2) If I really need the field to be mutable AND the class instance is
exposed more than one thread, then I would use an AtomicReference to make sure
it's thread-safe:
```
private final AtomicReference<NioSslEngine> sslEngine = new
AtomicReference<>();
```
Using an interface with a dummy implementation instead of null can also be
used with AtomicReferences.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
