[INTERNAL][CORE] Redo optimization to Socks5 transport #1074
Conversation
The current code was hard to understand and not really maintainable. Removed the simultaneous connection attempts in both directions. Only one connection attempt is made as is it very likely the the reverse connection attempt will also fail if the first one fails. The logic could be reintroduced but we should ensure that the connection attempt to B->A is made after A->B and not in parallel to keep the logic simple.
srossbach
added
Aspect: Network
|
Complexity decreasing per file
==============================
+ core/src/saros/net/stream/Socks5StreamService.java -4
See the complete overview on Codacy |
|
|
||
| Socks5BytestreamSession session = preferInSession ? inSession : outSession; | ||
| log.debug( |
There was a problem hiding this comment.
| log.debug( | |
| log.debug("establishing Socks5 bytestream to: {}, {}...", () -> remoteAddress, () -> verboseLocalProxyInfo()) |
There was a problem hiding this comment.
Guess that this is my opinion but I find it very frustrating that you simply cannot mix up lambdas and objects.
There was a problem hiding this comment.
I had the same expectation and was very confused about the error messages when I tried to mix both. However, I think it is still better than log4j-1.2.
| ((Socks5BytestreamRequest) request).setTotalConnectTimeout(TOTAL_CONNECT_TIMEOUT); | ||
| session = (Socks5BytestreamSession) request.accept(); | ||
| } catch (InterruptedException e) { | ||
| log.warn("interrupted while accepting request from: " + remoteAddress); |
There was a problem hiding this comment.
| log.warn("interrupted while accepting request from: " + remoteAddress); | |
| log.warn("interrupted while accepting request from: {}", remoteAddress); |
| // Thread.currentThread().interrupt(); | ||
| return; | ||
| } catch (XMPPException e) { | ||
| log.error("failed to accept request from from: " + remoteAddress, e); |
There was a problem hiding this comment.
| log.error("failed to accept request from from: " + remoteAddress, e); | |
| log.error("failed to accept request from from: {}", remoteAddress, e); |
There was a problem hiding this comment.
Isnt this swallowing the exception ?
There was a problem hiding this comment.
The exception handling is not very well documented, but the default exception handling is described in the layouts documentation (see alwaysWriteExceptions):
exceptions are always written even if the pattern contains no exception conversions. This means that if you do not include a way to output exceptions in your pattern, the default exception formatter will be added to the end of the pattern. Setting this to false disables this behavior and allows you to exclude exceptions from your pattern output.
| return establishBinaryChannel(connectionID, remoteAddress.toString()); | ||
| } catch (XMPPException e) { | ||
| throw new IOException(e); | ||
| log.error("failed to setup connection for request from: " + remoteAddress, e); |
There was a problem hiding this comment.
| log.error("failed to setup connection for request from: " + remoteAddress, e); | |
| log.error("failed to setup connection for request from: {}", remoteAddress, e); |
| @@ -819,93 +279,9 @@ private void configureSocks5Socket(Socks5BytestreamSession session) { | |||
|
|
|||
| try { | |||
| socket.setTcpNoDelay(TCP_NODELAY); | |||
| log.debug("nagle algorithm for socket disabled: " + TCP_NODELAY); | |||
| log.debug("Nagle algorithm for session " + session + " disabled: " + TCP_NODELAY); | |||
There was a problem hiding this comment.
| log.debug("Nagle algorithm for session " + session + " disabled: " + TCP_NODELAY); | |
| log.debug("Nagle algorithm for session {} disabled: {}", session, TCP_NODELAY); |
|
|
||
| if (listener == null) throw new IOException(this + " transport is not initialized"); | ||
| log.debug("received request to establish a Socks5 bytestream to: " + remoteAddress); |
There was a problem hiding this comment.
| log.debug("received request to establish a Socks5 bytestream to: " + remoteAddress); | |
| log.debug("received request to establish a Socks5 bytestream to: {}", remoteAddress); |
| log.warn( | ||
| "rejecting request from " + peer + " , no connection identifier found: " + sessionID); | ||
| if (currentConnectionListener == null || currentLocalAddress == null) { | ||
| log.warn("service is not initialized - rejecting request from: " + remoteAddress); |
There was a problem hiding this comment.
| log.warn("service is not initialized - rejecting request from: " + remoteAddress); | |
| log.warn("service is not initialized - rejecting request from: {}", remoteAddress); |
The current code was hard to understand and not really maintainable.
Removed the simultaneous connection attempts in both directions. Only
one connection attempt is made as is it very likely the the reverse
connection attempt will also fail if the first one fails.
The logic could be reintroduced but we should ensure that the connection
attempt to B->A is made after A->B and not in parallel to keep the logic
simple.