Switch to using Async for sending and recieving packets

[killeroo]

Switch to using Async for sending and recieving packets

[Killeroo]

Also switch listening to use async so we can check for exitEvent

[Killeroo]

Can we make sure that we are deffo using UDP to send packets?

[jdpurcell]

@Killeroo I was looking into this anyway for #34 and actually got it working, but I decided it wasn't worth it and just ended up using Task.Run to wrap the synchronous method. My findings:

• It increased the code complexity because the async receive methods never time out (documentation says SocketOptionName.ReceiveTimeout only applies to synchronous methods). It's easy enough to handle the timeout as part of the wait, but the problem is you cannot start a new request if the first one didn't finish. So in the case of a timeout, you have to remember that there is still an outstanding receive request and avoid starting a new one in the next loop iteration.
• It actually seems to have slightly worse latency than synchronous + Task.Run (tested with arguments --dp 3 127.0.0.1). The average latency with async was 0.080ms in my test vs 0.070ms for sync + Task.Run.
• The only good thing was that it didn't suck quite as bad in flood mode - faster and less CPU usage than sync + Task.Run, but still nowhere near as fast as fully synchronous. In my fix for #34 I use fully synchronous operations for timeouts of 250ms or less, so this point doesn't interest me.

Though to be fair, I was using Task.Factory.FromAsync to handle the begin/end calls, and I'm not sure what effect if any it had on performance.

Here's the extension method I came up with if you're interested:

public static Task<int> ReceiveFromAsync(this Socket socket, byte[] buffer, EndPoint remoteEP, Action<EndPoint> onEnd)
{
    IAsyncResult BeginMethod(AsyncCallback callback, object state) {
        return socket.BeginReceiveFrom(buffer, 0, buffer.Length, SocketFlags.None, ref remoteEP, callback, state);
    }
    int EndMethod(IAsyncResult asyncResult) {
        int bytesRead = socket.EndReceiveFrom(asyncResult, ref remoteEP);
        onEnd(remoteEP);
        return bytesRead;
    }
    return Task<int>.Factory.FromAsync(BeginMethod, EndMethod, null);
}

[jdpurcell]

I should do some more testing with the methods directly (without Task FromAsync) and see how it compares.

[Killeroo]

@jdpurcell Interesting results, thanks for looking into it. Yeah I think I originally thought that switching to async might be a good move because of #57 and because I saw the architecture of tools like MassScan which basically had one giant loop in a thread for receiving and another one for sending. But actually now I think about operations that PowerPing mostly does, it makes sense to stick with synchronous sending, because we are mostly just sending packets in sequential order.

I did think that there would be improvements for flooding, because we aren't so concerned with receiving packets after we have sent them, so having an architecture where we could easily decouple the receiving part of our code (such as seemed more clean with async). But I don't think we want to make really big architectural changes just for the benefit of one feature and the detriment of readability and efficiency in all other operations.

It makes me think that the synchronous system we have at the moment is in a pretty good spot. I would happy to see your results of synchronous vs synchronous with Task vs fully async, but your results have definitely helped inform the decision going forward.

I do think that at some point the sendICMP method should be refactored (Ping.cs and its operations could probably be split out too, but thats for later) because its doing alot of things that could be split off into self contained methods. But I want to hold off on big refactors till after releasing 1.3.0, because your fixes are addressing alot of core bugs that have been in PowerPing for too long. So maybe after releasing 1.3.0 (which I think we are in a good spot to do really soon) we can look into some refactoring but I think from your results I'm content with sticking to a synchronus model of handling packets as it suits the operation of the program (plus I think readability and complexity reduction is a big bonus).

[jdpurcell]

@Killeroo Interesting that you mention decoupling the receiving part, because I was thinking about that as well. I tried to parallelize the scan routine, which seems easy enough (Parallel.ForEach, for example), but the problem turns out to be that one thread can call ReceiveFrom and end up getting a response that should have gone to another thread. This seems obvious now, but I think I was misled about the way ReceiveFrom works. The "From" in the name and the fact that an EndPoint is passed in by "ref" instead of "out" almost seems to imply you can specify the IP you want to receive from and ignore everything else, but clearly that is not the case.

So the solution there would of course involve a single receive routine to coordinate monitoring the responses from multiple send threads.

EDIT: I thought the multiple threads (each with their own Socket instance) were somehow sharing a receive buffer, but after further testing I realize that's not what was happening. I was scanning my own subnet and when the scan reached my local IP, that must be a special case because the echo response went not only to the socket that sent the request, but to all other sockets that were waiting for a response as well. So the solution could be just to validate the remote address before counting it as a valid response.

[jdpurcell]

I think that's what was happening, at least. It seems like there is some "magic" going on with ReceiveFrom that I don't fully understand.

[Killeroo]

@jdpurcell Yeah if you are using the RecieveFrom from the same socket object along all the threads I can see that happening. If you look at the code for RecieveFrom it seems to try and overwrite the referrenced EndPoint with whatever address replies, I suppose its behavour that works fine on a single thread but can be a bit choppy when multiple threads are using one socket.

I have always been fairly annoyed with the performance of scan, taking over a minute to scan 255 addresses isn't really good enough compared to other scan tools. You could create a common recieve function to coordinate all the threads, or you could create a few threads each with their own ping object. I think I tried or started implementing the later (splitting up the addresses to be scanned for each of the threads) but its kind of a pain to get our synchronously designed Ping object to work in threads, always comes out a bit hacky.

[jdpurcell]

I tried the Begin/End methods directly but the performance was the same as using Task's FromAsync.

This is what the code ended up looking like if you're curious (based on my pr4 branch):

private PingResults SendICMP(PingAttributes attrs, Action<PingResults> onResultsUpdate = null)
{
    PingResults results = new PingResults();
    ICMP packet = new ICMP();
    byte[] receiveBuffer = new byte[attrs.RecieveBufferSize]; // Ipv4Header.length + IcmpHeader.length + attrs.recievebuffersize
    int bytesRead = 0, packetSize;

    // Convert to IPAddress
    IPAddress ipAddr = IPAddress.Parse(attrs.Address);

    // Setup endpoint
    IPEndPoint iep = new IPEndPoint(ipAddr, 0);

    // Setup raw socket
    Socket sock = CreateRawSocket(ipAddr.AddressFamily);

    // Set socket options
    sock.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.ReceiveTimeout, attrs.Timeout); // Socket timeout
    sock.SetSocketOption(SocketOptionLevel.IP, SocketOptionName.IpTimeToLive, attrs.Ttl);
    sock.DontFragment = attrs.DontFragment;

    // Create packet message payload
    byte[] payload;
    if (attrs.Size != -1) {
        payload = Helper.GenerateByteArray(attrs.Size);
    } else {
        payload = Encoding.ASCII.GetBytes(attrs.Message);
    }

    // Construct our ICMP packet
    packet.type = attrs.Type;
    packet.code = attrs.Code;
    Buffer.BlockCopy(BitConverter.GetBytes(sessionId), 0, packet.message, 0, 2); // Add identifier to ICMP message
    Buffer.BlockCopy(payload, 0, packet.message, 4, payload.Length); // Add text into ICMP message
    packet.messageSize = payload.Length + 4;
    packetSize = packet.messageSize + 4;

    ManualResetEventSlim receiveEvent = null;
    EndPoint responseEndPoint = iep;
    long responseTimestamp = 0;
    void BeginReceiveFrom() {
        receiveEvent = new ManualResetEventSlim();
        sock.BeginReceiveFrom(receiveBuffer, 0, receiveBuffer.Length, SocketFlags.None, ref responseEndPoint, ReceiveFromCallback, null);
    }
    void ReceiveFromCallback(IAsyncResult ar) {
        bytesRead = sock.EndReceiveFrom(ar, ref responseEndPoint);
        responseTimestamp = Stopwatch.GetTimestamp();
        receiveEvent.Set();
    }

    // Sending loop
    for (int index = 1; attrs.Continous || index <= attrs.Count; index++) {
        if (index != 1) {
            // Wait for set interval before sending again or cancel if requested
            if (cancellationToken.WaitHandle.WaitOne(attrs.Interval)) {
                break;
            }

            // Generate random interval when RandomTimings flag is set
            if (attrs.RandomTiming) {
                attrs.Interval = Helper.RandomInt(5000, 100000);
            }
        }

        // Include sequence number in ping message
        ushort sequenceNum = (ushort)index;
        Buffer.BlockCopy(BitConverter.GetBytes(sequenceNum), 0, packet.message, 2, 2);

        // Fill ICMP message field
        if (attrs.RandomMsg) {
            payload = Encoding.ASCII.GetBytes(Helper.RandomString());
            Buffer.BlockCopy(payload, 0, packet.message, 4, payload.Length);
        }

        // Update packet checksum
        packet.checksum = 0;
        UInt16 chksm = packet.GetChecksum();
        packet.checksum = chksm;

        try {
            // Show request packet
            if (Display.ShowRequests) {
                Display.RequestPacket(packet, Display.UseInputtedAddress | Display.UseResolvedAddress ? attrs.Host : attrs.Address, index);
            }

            // If there were extra responses from a prior request, ignore them
            if (receiveEvent != null && receiveEvent.IsSet) {
                receiveEvent = null;
            }
            if (receiveEvent == null) {
                while (sock.Available != 0) {
                    bytesRead = sock.Receive(receiveBuffer);
                }
            }

            // Send ping request
            sock.SendTo(packet.GetBytes(), packetSize, SocketFlags.None, iep); // Packet size = message field + 4 header bytes
            long requestTimestamp = Stopwatch.GetTimestamp();
            try { results.Sent++; }
            catch (OverflowException) { results.HasOverflowed = true; }

            if (debug) {
                // Induce random wait for debugging
                Random rnd = new Random();
                Thread.Sleep(rnd.Next(700));
                if (rnd.Next(20) == 1) { throw new SocketException(); }
            }

            ICMP response;
            TimeSpan replyTime = TimeSpan.Zero;
            do {
                // Wait for response
                if (receiveEvent == null) {
                    BeginReceiveFrom();
                }
                if (!receiveEvent.Wait(attrs.Timeout, cancellationToken)) {
                    throw new SocketException();
                }
                receiveEvent = null;
                replyTime = new TimeSpan(Helper.StopwatchToTimeSpanTicks(responseTimestamp - requestTimestamp));

                // Store reply packet
                response = new ICMP(receiveBuffer, bytesRead);

                // Ignore unexpected echo responses
                if (packet.type == 8 && response.type == 0) {
                    ushort responseSessionId = BitConverter.ToUInt16(response.message, 0);
                    ushort responseSequenceNum = BitConverter.ToUInt16(response.message, 2);
                    if (responseSessionId != sessionId || responseSequenceNum != sequenceNum) {
                        if (replyTime.TotalMilliseconds >= attrs.Timeout) {
                            throw new SocketException();
                        }
                        response = null;
                    }
                }
            } while (response == null);

            // Display reply packet
            if (Display.ShowReplies) {
                PowerPing.Display.ReplyPacket(response, Display.UseInputtedAddress | Display.UseResolvedAddress ? attrs.Host : responseEndPoint.ToString(), index, replyTime, bytesRead);
            }

            // Store response info
            try { results.Received++; }
            catch (OverflowException) { results.HasOverflowed = true; }
            results.CountPacketType(response.type);
            results.SaveResponseTime(replyTime.TotalMilliseconds);

            if (attrs.BeepLevel == 2) {
                try { Console.Beep(); }
                catch (Exception) { } // Silently continue if Console.Beep errors
            }
        } catch (IOException) {
            if (Display.ShowOutput) {
                PowerPing.Display.Error("General transmit error");
            }
            results.SaveResponseTime(-1);
            try { results.Lost++; }
            catch (OverflowException) { results.HasOverflowed = true; }
        } catch (SocketException) {
            PowerPing.Display.Timeout(index);
            if (attrs.BeepLevel == 1) {
                try { Console.Beep(); }
                catch (Exception) { results.HasOverflowed = true; }
            }
            results.SaveResponseTime(-1);
            try { results.Lost++; }
            catch (OverflowException) { results.HasOverflowed = true; }
        } catch (OperationCanceledException) {
            break;
        } catch (Exception) {
            if (Display.ShowOutput) {
                PowerPing.Display.Error("General error occured");
            }
            results.SaveResponseTime(-1);
            try { results.Lost++; }
            catch (OverflowException) { results.HasOverflowed = true; }
        }

        onResultsUpdate?.Invoke(results);
    }

    // Clean up
    sock.Close();

    return results;
}