[INTERNAL][CORE] moves Socks5Logic out of XMPPConnection #869

srossbach · 2020-03-08T01:05:51Z

The whole logic regarding the Socks5Proxy is now handled in its own
class.

srossbach · 2020-03-08T01:08:13Z

core/src/saros/net/xmpp/Socks5ProxySupport.java

+
+    socks5Proxy = null;
+
+    if (socks5ProxyPort < 0) return;


core/src/saros/net/xmpp/Socks5ProxySupport.java

m273d15 · 2020-03-09T08:57:23Z

core/src/saros/net/xmpp/Socks5ProxySupport.java

+    final Socks5Proxy newSocks5Proxy = Socks5Proxy.getSocks5Proxy();
+    socks5Proxy = newSocks5Proxy;


Suggested change

final Socks5Proxy newSocks5Proxy = Socks5Proxy.getSocks5Proxy();

socks5Proxy = newSocks5Proxy;

socks5Proxy = Socks5Proxy.getSocks5Proxy();

Do you need the "newSocks5Proxy" variable? It is used by l. 137, l. 159. I see no need for an additional variable.

I have not found any information if lambdas act as closures nor not. The lambda should definitely not access the socks5Proxy variable as it can already be null inside the threads.

Do you mean

saros/core/src/saros/net/xmpp/Socks5ProxySupport.java

Lines 127 to 132 in 6655c32

ThreadUtils.runSafeAsync(

"saros-stun-discovery",

log,

() -> {

discoverAndPublishStunAddresses(socks5Proxy, stunServerAddress, stunServerPort);

});

?
In this case socks5Proxy is used from the heap, so formally this is a concurrency problem which is unlikely (disableProxy() would need to run before...).

core/src/saros/net/xmpp/Socks5ProxySupport.java

m273d15 · 2020-03-09T09:05:47Z

core/src/saros/net/xmpp/Socks5ProxySupport.java

+      return;
+    }
+
+    Socks5Proxy.getSocks5Proxy().stop();


Would lead to a NPE if called before the "enableProxy" method. I don't know whether it is worth to check for null.

This was a C&P, socks5Proxy.stop() is the correct call.

core/src/saros/net/xmpp/Socks5ProxySupport.java

m273d15 · 2020-03-09T09:29:11Z

core/src/saros/net/xmpp/Socks5ProxySupport.java

+  private static void addSocks5ProxyAddresses(
+      final Socks5Proxy proxy, final Collection<String> addresses, boolean inFront) {
+
+    synchronized (socks5AddressReplacementLock) {


Just for my own understanding: I would assume that you could make the method synchronized without the extra object, because synchronized <methodname> locks this and synchronized static locks <ClassName>.class. Is this true?

Yes I could also synchronize on the class variable. I could also make this method non static however it should not use synchronized(this) then.

core/src/saros/net/xmpp/Socks5ProxySupport.java

tobous · 2020-03-18T11:34:59Z

core/src/saros/net/xmpp/Socks5ProxySupport.java

+
+      if (!upnpService.isMapped(device, socks5ProxyPort, IUPnPService.TCP)) return;
+
+      if (!upnpService.deletePortMapping(device, socks5ProxyPort, IUPnPService.TCP)) {


Move call out of if condition to make it clearer that it actually has an effect?

Suggested change

if (!upnpService.deletePortMapping(device, socks5ProxyPort, IUPnPService.TCP)) {

boolean portMappingDeletionSuccessful =

upnpService.deletePortMapping(device, socks5ProxyPort, IUPnPService.TCP);

if (!portMappingDeletionSuccessful) {

core/src/saros/net/xmpp/Socks5ProxySupport.java

tobous · 2020-03-18T11:54:41Z

core/src/saros/net/xmpp/Socks5ProxySupport.java

+      }
+    }
+
+    try {


This block only tears down the upnp service. But during the setup, you also initialized the stun service. Does it not require a "manual" teardown?

The Stun service does only lookup some stuff while the UPNP stuff may setup a permanent port mapping. I.e the retrieved addresses goes to no longer valid socks5 proxy instance and therefore it does not matter. I can also change the code to not even wait for the stun process to finish at all, it would not matter.

tobous · 2020-03-18T12:01:47Z

core/src/saros/net/xmpp/Socks5ProxySupport.java

+
+    upnpService.deletePortMapping(device, socks5ProxyPort, IUPnPService.TCP);
+
+    if (!upnpService.createPortMapping(


Move call that actually has an effect out of if condition?

tobous · 2020-03-18T12:11:33Z

core/src/saros/net/xmpp/Socks5ProxySupport.java

+   * @return <code>true</code> if the proxy was successfully started, <code>false</code> if it could
+   *     not be started or is already running
+   */
+  public synchronized boolean enableProxy(


The old logic called socks5Proxy.start() in the setup. Is such a call no longer necessary?

Sames goes for socks5Proxy.stop() in the teardown.

The socks 5 proxy will automatically start if you call getSocks5Proxy() after setting setLocalSocks5ProxyEnabled(true)

I still have I hard time as the Smack source code is no longer available and I cannot find the source anymore for the version we are currently using.

The src is still available https://github.com/igniterealtime/Smack/tree/3.4.1

core/src/saros/net/xmpp/Socks5ProxySupport.java

stefaus · 2020-06-23T09:01:05Z

core/src/saros/net/xmpp/Socks5ProxySupport.java

+            + socks5Proxy.getPort()
+            + " [listening on all interfaces]");
+
+    final List<String> proxyAddressesToPublish = new ArrayList<>();


Any reason to not directly assign the collected / copied lists?

Because proxyAddresses is a collection and not a list.

?
you could do proxyAddressesToPublish = new ArrayList<>(proxyAddresses);

core/src/saros/net/xmpp/Socks5ProxySupport.java

The whole logic regarding the Socks5Proxy is now handled in its own class. In addition only the STUN discovery is performed asynchronous. Assuming the UPNP router works as expected mapping and unmapping ports just takes a few milliseconds.

saros-infrastructure · 2020-06-23T18:38:02Z

Here is an overview of what got changed by this pull request:

Complexity increasing per file
==============================
- core/src/saros/net/xmpp/Socks5ProxySupport.java  17
         

Complexity decreasing per file
==============================
+ core/src/saros/net/xmpp/XMPPConnectionService.java  -3

See the complete overview on Codacy

m273d15 · 2020-06-25T14:08:26Z

@srossbach Is this ready to review?

srossbach · 2020-06-25T14:25:29Z

Yes.

tobous · 2020-06-26T14:58:29Z

core/src/saros/net/xmpp/Socks5ProxySupport.java

+      return;
+    }
+
+    log.info(


Move to else block and remove return from if to improve readability/make code paths easier to understand?

tobous · 2020-06-26T14:59:58Z

core/src/saros/net/xmpp/Socks5ProxySupport.java

+              + "]");
+    }
+
+    log.info(


Same as above. Move to else block?

Otherwise, you should add a return statement to the if block as currently the "successful" message is still also displayed on failure.

srossbach requested review from m273d15, stefaus and tobous March 8, 2020 01:05

srossbach commented Mar 8, 2020

View reviewed changes

core/src/saros/net/xmpp/Socks5ProxySupport.java Outdated

socks5Proxy = null;

if (socks5ProxyPort < 0) return;

Copy link

Contributor Author

srossbach Mar 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<=

m273d15 reviewed Mar 9, 2020

View reviewed changes

tobous reviewed Mar 18, 2020

View reviewed changes

stefaus reviewed Jun 23, 2020

View reviewed changes

srossbach force-pushed the pr/socks_5_access_refactor branch from 1a59ae7 to 6655c32 Compare June 23, 2020 18:35

tobous reviewed Jun 26, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[INTERNAL][CORE] moves Socks5Logic out of XMPPConnection #869

[INTERNAL][CORE] moves Socks5Logic out of XMPPConnection #869

srossbach commented Mar 8, 2020

srossbach Mar 8, 2020

m273d15 Mar 9, 2020

srossbach Mar 9, 2020

stefaus Jun 25, 2020

m273d15 Mar 9, 2020

srossbach Mar 9, 2020

m273d15 Mar 9, 2020

srossbach Mar 9, 2020

tobous Mar 18, 2020 •

edited

Loading

tobous Mar 18, 2020

srossbach Mar 18, 2020

tobous Mar 18, 2020

tobous Mar 18, 2020

srossbach Mar 18, 2020

stefaus Jun 25, 2020

stefaus Jun 23, 2020

srossbach Jun 23, 2020

stefaus Jun 25, 2020

saros-infrastructure commented Jun 23, 2020

m273d15 commented Jun 25, 2020

srossbach commented Jun 25, 2020

tobous Jun 26, 2020

tobous Jun 26, 2020

		final Socks5Proxy newSocks5Proxy = Socks5Proxy.getSocks5Proxy();
		socks5Proxy = newSocks5Proxy;

	final Socks5Proxy newSocks5Proxy = Socks5Proxy.getSocks5Proxy();
	socks5Proxy = newSocks5Proxy;
	socks5Proxy = Socks5Proxy.getSocks5Proxy();

	ThreadUtils.runSafeAsync(
	"saros-stun-discovery",
	log,
	() -> {
	discoverAndPublishStunAddresses(socks5Proxy, stunServerAddress, stunServerPort);
	});


		if (!upnpService.isMapped(device, socks5ProxyPort, IUPnPService.TCP)) return;

		if (!upnpService.deletePortMapping(device, socks5ProxyPort, IUPnPService.TCP)) {

+                    }
+                  }
+                  try {


		upnpService.deletePortMapping(device, socks5ProxyPort, IUPnPService.TCP);

		if (!upnpService.createPortMapping(

[INTERNAL][CORE] moves Socks5Logic out of XMPPConnection #869

Are you sure you want to change the base?

[INTERNAL][CORE] moves Socks5Logic out of XMPPConnection #869

Conversation

srossbach commented Mar 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobous Mar 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saros-infrastructure commented Jun 23, 2020

m273d15 commented Jun 25, 2020

srossbach commented Jun 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobous Mar 18, 2020 •

edited

Loading