Fix for issue #103 infinite loop seed generation

.vscode/settings.json

@@ -0,0 +1,7 @@
+{
+    "python.testing.pytestArgs": [
+        "test"
+    ],
+    "python.testing.unittestEnabled": false,
+    "python.testing.pytestEnabled": true
+}

CONTRIBUTING.md

@@ -6,12 +6,11 @@ here on GitHub.
 Please note we have a code of conduct, please follow it in all your interactions with the project.
 
 ## Pull Request Process
+** All PRs must merge into the dev branch, and not into master**. The dev branch is the only one that can merge into master directly.
 
 1. Ensure any install or build dependencies are removed before the end of the layer when doing a 
    build.
-2. Update the README.md with details of changes to the interface, this includes new environment 
+2. Make sure all corresponding test cases pass.
+3. Update the README.md with details of changes to the interface, this includes new environment 
    variables, exposed ports, useful file locations and container parameters.
-3. Increase the version numbers in any examples files and the README.md to the new version that this
-   Pull Request would represent. The versioning scheme we use is [SemVer](http://semver.org/).
-4. You may merge the Pull Request in once you have the sign-off of two other developers, or if you 
-   do not have permission to do that, you may request the second reviewer to merge it for you.
+4. Version number will be increased only when merging dev into master, so that a version update may carry multiple PRs from various developers. The versioning scheme we use is [SemVer](http://semver.org/).

README.md

@@ -71,3 +71,91 @@ which payloads to send to the remote device. For that, we have several classes i
 `payload_provider` module. You may want to create your own provider by extending
 `payload_provider.PayloadProvider`. If you are interested in that, you should check the
 documentation of both `executor` and `payload_provider` module.
+
+## Code Structure
+
+### Top Level Directory Layout
+Our project directory structure contains all src files in the pythonping folder, test cases in another folder, and helping documentation in on the top level directory.
+
+```
+.
+├── pythonping              # Source files 
+├── test                    # Automated Testcases for the package
+├── CODE_OF_CONDUCT         # An md file containing code of conduct
+├── CONTRIBUTING            # Contributing Guidlins
+├── LICENSE                 # MIT License
+├── README.md               # An md file
+└── setup.py                # Instalation
+```
+
+A UML Diagram of the code structure is below:
+
+![ER1](https://raw.githubusercontent.com/alessandromaggio/pythonping/master/docs/UML-Diagram.png)
+
+As per the uml diagram above five distinct classes outside of init exist in this package: Executor, Icmp, Payload Provider, and Utils. Each of them rely on attributes which have been listed as sub-classes for brevities sake. An overview of each class is as follows.
+
+### Utils 
+Simply generates random text. See function random_text.
+
+### Network 
+Opens a socket to send and recive data. See functions send, recv, and del.
+
+### Payload Provider
+Generates ICMP Payloads with no Headers. It's functionaly a interface. It has three
+functions init, iter, and next, which are all implmented by subclasses List, Repeat, and Sweep which store payloads in diffrent lists.
+
+### ICMP
+Generates the ICMP heaser through subclass ICMPType, and various helper functions.
+
+### Executor
+Has various subclasses including Message, Response, Success, and Communicator used for sending icmp packets and collecting data.
+
+### Init
+Uses network, executor, payload_provider and utils.random_text to construct and send ICMP packets to ping a network. 
+
+## Tests
+A test package exists under the folder test, and contains a serise of unit tests. Before commiting changes make sure to run the test bench and make sure all corrisponding cases pass. For new functionality new test cases must be added and documented.
+
+To run testcases we can simply use the ```unitest discover``` utility by running the following command:
+
+```
+python -m unittest discover <test_directory>
+```
+
+To run the test cases in a specific file FILE we must run the following command:
+
+```
+python -m unittest discover -s <test_directory> -p FILE
+```
+
+Another option is to run the following from the top level directory:
+
+```
+pytest test
+```
+
+To test for coverage simply run:
+
+```
+coverage run -m pytest test
+```
+
+## Contributing
+Before contributing read through the contribution guidlines found the CONTRIBUTING file. 
+
+### Code Style
+A few key points when contributing to this repo are as follows:
+1. Use tabs over spaces.
+2. Format doc strings as such:
+    ```
+    DESCRIPTION
+
+        :param X: DESCRIPTION
+        :type X: Type
+        :param Y: DESCRIPTION
+        :type Y: Type
+    ```
+    Please add doc strings to all functions added.
+3. Do not add spaces between docstring and first function line.
+4. Do not go over 200 characters per line.
+5. When closing multiline items under brackets('()', '[]', ... etc) put the closing bracket on it's own line. 

pythonping/__init__.py

@@ -1,7 +1,7 @@
 import sys
+from random import randint
 from . import network, executor, payload_provider
 from .utils import random_text
-from random import randint
 
 
 # this needs to be available across all thread usages and will hold ints
@@ -68,21 +68,24 @@ def ping(target,
     if df:
         options = network.Socket.DONT_FRAGMENT
 
-    # Fix to allow for pythonping multithreaded usage;
-    # no need to protect this loop as no one will ever surpass 0xFFFF amount of threads
-    while True:
-        # seed_id needs to be less than or equal to 65535 (as original code was seed_id = getpid() & 0xFFFF)
-        seed_id = randint(0x1, 0xFFFF)
-        if seed_id not in SEED_IDs:
-            SEED_IDs.append(seed_id)
-            break
+    try:
+        # Fix to allow for pythonping multithreaded usage;
+        # no need to protect this loop as no one will ever surpass 0xFFFF amount of threads
+        while True:
+            # seed_id needs to be less than or equal to 65535 (as original code was seed_id = getpid() & 0xFFFF)
+            seed_id = randint(0x1, 0xFFFF)
+            if seed_id not in SEED_IDs:
+                SEED_IDs.append(seed_id)
+                break
 
 
-    comm = executor.Communicator(target, provider, timeout, interval, socket_options=options, verbose=verbose, output=out,
-                                 seed_id=seed_id, source=source, repr_format=out_format)
+        comm = executor.Communicator(target, provider, timeout, interval, socket_options=options, verbose=verbose, output=out,
+                                    seed_id=seed_id, source=source, repr_format=out_format)
 
-    comm.run(match_payloads=match)
+        comm.run(match_payloads=match)
 
-    SEED_IDs.remove(seed_id)
+    finally:
+        if seed_id in SEED_IDs:
+            SEED_IDs.remove(seed_id)
 
     return comm.responses

pythonping/executor.py

@@ -92,35 +92,34 @@ def success(self):
     def error_message(self):
         if self.message is None:
             return 'No response'
-        else:
-            if self.message.packet.message_type == 0 and self.message.packet.message_code == 0:
-                # Echo Reply, response OK - no error
-                return None
-            elif self.message.packet.message_type == 3:
-                # Destination unreachable, returning more details based on message code
-                unreachable_messages = [
-                    'Network Unreachable',
-                    'Host Unreachable',
-                    'Protocol Unreachable',
-                    'Port Unreachable',
-                    'Fragmentation Required',
-                    'Source Route Failed',
-                    'Network Unknown',
-                    'Host Unknown',
-                    'Source Host Isolated',
-                    'Communication with Destination Network is Administratively Prohibited',
-                    'Communication with Destination Host is Administratively Prohibited',
-                    'Network Unreachable for ToS',
-                    'Host Unreachable for ToS',
-                    'Communication Administratively Prohibited',
-                    'Host Precedence Violation',
-                    'Precedence Cutoff in Effect'
-                ]
-                try:
-                    return unreachable_messages[self.message.packet.message_code]
-                except IndexError:
-                    # Should never generate IndexError, this serves as additional protection
-                    return 'Unreachable'
+        if self.message.packet.message_type == 0 and self.message.packet.message_code == 0:
+            # Echo Reply, response OK - no error
+            return None
+        if self.message.packet.message_type == 3:
+            # Destination unreachable, returning more details based on message code
+            unreachable_messages = [
+                'Network Unreachable',
+                'Host Unreachable',
+                'Protocol Unreachable',
+                'Port Unreachable',
+                'Fragmentation Required',
+                'Source Route Failed',
+                'Network Unknown',
+                'Host Unknown',
+                'Source Host Isolated',
+                'Communication with Destination Network is Administratively Prohibited',
+                'Communication with Destination Host is Administratively Prohibited',
+                'Network Unreachable for ToS',
+                'Host Unreachable for ToS',
+                'Communication Administratively Prohibited',
+                'Host Precedence Violation',
+                'Precedence Cutoff in Effect'
+            ]
+            try:
+                return unreachable_messages[self.message.packet.message_code]
+            except IndexError:
+                # Should never generate IndexError, this serves as additional protection
+                return 'Unreachable'
         # Error was not identified
         return 'Network Error'
 
@@ -131,28 +130,26 @@ def time_elapsed_ms(self):
     def legacy_repr(self):
         if self.message is None:
             return 'Request timed out'
-        elif self.success:
+        if self.success:
             return 'Reply from {0}, {1} bytes in {2}ms'.format(self.message.source,
                                                                len(self.message.packet.raw),
                                                                self.time_elapsed_ms)
-        else:
-            # Not successful, but with some code (e.g. destination unreachable)
-            return '{0} from {1} in {2}ms'.format(self.error_message, self.message.source, self.time_elapsed_ms)
+        # Not successful, but with some code (e.g. destination unreachable)
+        return '{0} from {1} in {2}ms'.format(self.error_message, self.message.source, self.time_elapsed_ms)
 
     def __repr__(self):
         if self.repr_format == 'legacy':
             return self.legacy_repr()
         if self.message is None:
             return 'Timed out'
-        elif self.success:
+        if self.success:
             return 'status=OK\tfrom={0}\tms={1}\t\tbytes\tsnt={2}\trcv={3}'.format(
                 self.message.source,
                 self.time_elapsed_ms,
                 len(self.source_request.raw)+20,
                 len(self.message.packet.raw)
             )
-        else:
-            return 'status=ERR\tfrom={1}\terror="{0}"'.format(self.message.source, self.error_message)
+        return 'status=ERR\tfrom={1}\terror="{0}"'.format(self.message.source, self.error_message)
 
 class ResponseList:
     """Represents a series of ICMP responses"""
@@ -231,23 +228,28 @@ def append(self, value):
                 self.rtt_max = value.time_elapsed
             if value.time_elapsed < self.rtt_min:
                 self.rtt_min = value.time_elapsed
-        if value.success: self.stats_packets_returned += 1
+        if value.success:
+            self.stats_packets_returned += 1
 
         if self.verbose:
             print(value, file=self.output)
 
     @property
-    def stats_packets_lost(self): return self.stats_packets_sent - self.stats_packets_returned
+    def stats_packets_lost(self):
+        return self.stats_packets_sent - self.stats_packets_returned
 
     @property
-    def stats_success_ratio(self): return self.stats_packets_returned / self.stats_packets_sent
+    def stats_success_ratio(self):
+        return self.stats_packets_returned / self.stats_packets_sent
 
     @property
-    def stats_lost_ratio(self): return 1 - self.stats_success_ratio
+    def stats_lost_ratio(self):
+        return 1 - self.stats_success_ratio
 
     @property
-    def packets_lost(self): return self.stats_lost_ratio
-
+    def packets_lost(self):
+        return self.stats_lost_ratio
+
     def __len__(self):
         return len(self._responses)