Update UPnP documentation

Adds more details, especially about caveats, failure modes and pitfalls

2 files changed, 21 insertions, 12 deletions

diff --git a/modules/upnp/doc_classes/UPNP.xml b/modules/upnp/doc_classes/UPNP.xml
index 847110abd4..4888dca822 100644
--- a/modules/upnp/doc_classes/UPNP.xml
+++ b/modules/upnp/doc_classes/UPNP.xml
@@ -1,16 +1,15 @@
 <?xml version="1.0" encoding="UTF-8" ?>
 <class name="UPNP" inherits="RefCounted" version="4.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="../../../doc/class.xsd">
 	<brief_description>
-		UPNP network functions.
+		Universal Plug and Play (UPnP) functions for network device discovery, querying and port forwarding.
 	</brief_description>
 	<description>
-		Provides UPNP functionality to discover [UPNPDevice]s on the local network and execute commands on them, like managing port mappings (port forwarding) and querying the local and remote network IP address. Note that methods on this class are synchronous and block the calling thread.
-		To forward a specific port:
+		This class can be used to discover compatible [UPNPDevice]s on the local network and execute commands on them, like managing port mappings (for port forwarding/NAT traversal) and querying the local and remote network IP address. Note that methods on this class are synchronous and block the calling thread.
+		To forward a specific port (here [code]7777[/code], note both [method discover] and [method add_port_mapping] can return errors that should be checked):
 		[codeblock]
-		const PORT = 7777
 		var upnp = UPNP.new()
-		upnp.discover(2000, 2, "InternetGatewayDevice")
-		upnp.add_port_mapping(port)
+		upnp.discover()
+		upnp.add_port_mapping(7777)
 		[/codeblock]
 		To close a specific port (e.g. after you have finished using it):
 		[codeblock]
@@ -21,7 +20,7 @@
 		# Emitted when UPnP port mapping setup is completed (regardless of success or failure).
 		signal upnp_completed(error)
 
-		# Replace this with your own server port number between 1025 and 65535.
+		# Replace this with your own server port number between 1024 and 65535.
 		const SERVER_PORT = 3928
 		var thread = null
 
@@ -48,6 +47,14 @@
 		    # Wait for thread finish here to handle game exit while the thread is running.
 		    thread.wait_to_finish()
 		[/codeblock]
+		[b]Terminology:[/b] In the context of UPnP networking, "gateway" (or "internet gateway device", short IGD) refers to network devices that allow computers in the local network to access the internet ("wide area network", WAN). These gateways are often also called "routers".
+		[b]Pitfalls:[/b]
+		- As explained above, these calls are blocking and shouldn't be run on the main thread, especially as they can block for multiple seconds at a time. Use threading!
+		- Networking is physical and messy. Packets get lost in transit or get filtered, addresses, free ports and assigned mappings change, and devices may leave or join the network at any time. Be mindful of this, be diligent when checking and handling errors, and handle these gracefully if you can: add clear error UI, timeouts and re-try handling.
+		- Port mappings may change (and be removed) at any time, and the remote/external IP address of the gateway can change likewise. You should consider re-querying the external IP and try to update/refresh the port mapping periodically (for example, every 5 minutes and on networking failures).
+		- Not all devices support UPnP, and some users disable UPnP support. You need to handle this (e.g. documenting and requiring the user to manually forward ports, or adding alternative methods of NAT traversal, like a relay/mirror server, or NAT hole punching, STUN/TURN, etc.).
+		- Consider what happens on mapping conflicts. Maybe multiple users on the same network would like to play your game at the same time, or maybe another application uses the same port. Make the port configurable, and optimally choose a port automatically (re-trying with a different port on failure).
+		[b]Further reading:[/b] If you want to know more about UPnP (and the Internet Gateway Device (IGD) and Port Control Protocol (PCP) specifically), [url=https://en.wikipedia.org/wiki/Universal_Plug_and_Play]Wikipedia[/url] is a good first stop, the specification can be found at the [url=https://openconnectivity.org/developer/specifications/upnp-resources/upnp/]Open Connectivity Foundation[/url] and Godot's implementation is based on the [url=https://github.com/miniupnp/miniupnp]MiniUPnP client[/url].
 	</description>
 	<tutorials>
 	</tutorials>
@@ -67,9 +74,11 @@
 			<param index="3" name="proto" type="String" default="&quot;UDP&quot;" />
 			<param index="4" name="duration" type="int" default="0" />
 			<description>
-				Adds a mapping to forward the external [code]port[/code] (between 1 and 65535) on the default gateway (see [method get_gateway]) to the [code]internal_port[/code] on the local machine for the given protocol [code]proto[/code] (either [code]TCP[/code] or [code]UDP[/code], with UDP being the default). If a port mapping for the given port and protocol combination already exists on that gateway device, this method tries to overwrite it. If that is not desired, you can retrieve the gateway manually with [method get_gateway] and call [method add_port_mapping] on it, if any.
+				Adds a mapping to forward the external [code]port[/code] (between 1 and 65535, although recommended to use port 1024 or above) on the default gateway (see [method get_gateway]) to the [code]internal_port[/code] on the local machine for the given protocol [code]proto[/code] (either [code]TCP[/code] or [code]UDP[/code], with UDP being the default). If a port mapping for the given port and protocol combination already exists on that gateway device, this method tries to overwrite it. If that is not desired, you can retrieve the gateway manually with [method get_gateway] and call [method add_port_mapping] on it, if any. Note that forwarding a well-known port (below 1024) with UPnP may fail depending on the device.
+				Depending on the gateway device, if a mapping for that port already exists, it will either be updated or it will refuse this command due to that conflict, especially if the existing mapping for that port wasn't created via UPnP or points to a different network address (or device) than this one.
 				If [code]internal_port[/code] is [code]0[/code] (the default), the same port number is used for both the external and the internal port (the [code]port[/code] value).
-				The description ([code]desc[/code]) is shown in some router UIs and can be used to point out which application added the mapping. The mapping's lease duration can be limited by specifying a [code]duration[/code] (in seconds). However, some routers are incompatible with one or both of these, so use with caution and add fallback logic in case of errors to retry without them if in doubt.
+				The description ([code]desc[/code]) is shown in some routers management UIs and can be used to point out which application added the mapping.
+				The mapping's lease [code]duration[/code] can be limited by specifying a duration in seconds. The default of [code]0[/code] means no duration, i.e. a permanent lease and notably some devices only support these permanent leases. Note that whether permanent or not, this is only a request and the gateway may still decide at any point to remove the mapping (which usually happens on a reboot of the gateway, when its external IP address changes, or on some models when it detects a port mapping has become inactive, i.e. had no traffic for multiple minutes). If not [code]0[/code] (permanent), the allowed range according to spec is between [code]120[/code] (2 minutes) and [code]86400[/code] seconds (24 hours).
 				See [enum UPNPResult] for possible return values.
 			</description>
 		</method>
@@ -84,7 +93,7 @@
 			<param index="0" name="port" type="int" />
 			<param index="1" name="proto" type="String" default="&quot;UDP&quot;" />
 			<description>
-				Deletes the port mapping for the given port and protocol combination on the default gateway (see [method get_gateway]) if one exists. [code]port[/code] must be a valid port between 1 and 65535, [code]proto[/code] can be either [code]TCP[/code] or [code]UDP[/code]. See [enum UPNPResult] for possible return values.
+				Deletes the port mapping for the given port and protocol combination on the default gateway (see [method get_gateway]) if one exists. [code]port[/code] must be a valid port between 1 and 65535, [code]proto[/code] can be either [code]TCP[/code] or [code]UDP[/code]. May be refused for mappings pointing to addresses other than this one, for well-known ports (below 1024), or for mappings not added via UPnP. See [enum UPNPResult] for possible return values.
 			</description>
 		</method>
 		<method name="discover">
diff --git a/modules/upnp/doc_classes/UPNPDevice.xml b/modules/upnp/doc_classes/UPNPDevice.xml
index b599acaba2..538ca72391 100644
--- a/modules/upnp/doc_classes/UPNPDevice.xml
+++ b/modules/upnp/doc_classes/UPNPDevice.xml
@@ -1,10 +1,10 @@
 <?xml version="1.0" encoding="UTF-8" ?>
 <class name="UPNPDevice" inherits="RefCounted" version="4.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="../../../doc/class.xsd">
 	<brief_description>
-		UPNP device.
+		Universal Plug and Play (UPnP) device.
 	</brief_description>
 	<description>
-		UPNP device. See [UPNP] for UPNP discovery and utility functions. Provides low-level access to UPNP control commands. Allows to manage port mappings (port forwarding) and to query network information of the device (like local and external IP address and status). Note that methods on this class are synchronous and block the calling thread.
+		Universal Plug and Play (UPnP) device. See [UPNP] for UPnP discovery and utility functions. Provides low-level access to UPNP control commands. Allows to manage port mappings (port forwarding) and to query network information of the device (like local and external IP address and status). Note that methods on this class are synchronous and block the calling thread.
 	</description>
 	<tutorials>
 	</tutorials>