Problem with ReadTerminated when using newline, \r\n

[co2chicken]

Hello-

First, thanks for the great, simple piece of code. I am having some difficulty and I'm not sure the best way to go about fixing it, so I've opened this issue.

I'm trying to read lines using the client. However, client.ReadTerminated("\r\n") doesn't work. This is because in your "IsTerminatorLocated" method in BaseClient.cs, you use
return s.TrimEnd().EndsWith(terminator);

I'd suggest changing this to:
return s.Contains(terminator);

First, TrimEnd() removes the \n\r from string, so EndsWith can't find the newline. Also, it's possible that a non-whitespace character ends up after your terminator (e.g. "> *").

Thanks,
Ben

[9swampy]

The intention is to allow you to specifically anticipate the last character that indicates the telnet server is awaiting further input, in your example that would be the "> *" prompt. Can you not set the terminator to be "> *"?

If the response really is terminating with the line feed then is the text that comes before the "\r\n" not a stable string that could be anticipated?

[9swampy]

Ok, something I'd thought about but never implemented as I hadn't needed it myself is an overload for the TerminatedRead that would accept a regular expression and return true upon RegEx.IsMatch. Perhaps that will help you achieve what you needed, if the comments above didn't help. Note there is a peculiar limitation of regular expressions which make handling your /r/n more difficult than it otherwise needed; see http://stackoverflow.com/questions/8618557/why-doesnt-in-net-multiline-regular-expressions-match-crlf

I've implemented, pushed to the repo and published an updated NuGet. HTH. Let me know if you're still stuck...

[co2chicken]

Yes, the text that comes before the "\r\n" is not a stable string. For some reason, there is no prompt character in this device. So the best way to know a line has been output is to look for the "\r\n" terminator.

I appreciate the effort to make a new method; However, the best option for me was to just make my own ReadLine() method, which looks for .contains("\r\n") instead of trim().endswith().

[co2chicken]

On an unrelated issue, I spent quite a while trying to access some sort of awaitable .ReadAsync() method. Cpu use by my thread was at 50% when I call .Read(). I had to add a Task.Delay() between .Read() calls to keep processor load low.

[9swampy]

Fair enough, if it works best for you go with it. Obviously I don't want to go and make a breaking change in the existing method, I'll only point out that by my understanding of your requirement passing Regex regex = new Regex(".*\r?$", RegexOptions.Multiline); into the added method should achieve the same end as your .contains("\r\n") but in a more generic way that may prove useful to others.

On the unrelated issue if you post a working example of the problem call I'll have a look at what was going wrong and see if I can help. I suspect it's likely however that you called async Task<string> TerminatedReadAsync(Regex regex, TimeSpan timeout) which relays up to async Task<string> TerminatedReadAsync(Regex regex, TimeSpan timeout, int millisecondSpin) filling in a default value of 1 for millisecondSpin. A larger millisecondSpin may be appropriate and would be equivalent to your interjection of Task.Delay()?

[co2chicken]

Thanks for volunteering to look at this code.

Issue #1 - I can't access the async methods, like TerminateReadAsync. It just isn't suggested by intellisense and doesn't compile if manually entered. I'm using .NET4.0 and have microsoft.bcl.async added. So I use TerminateRead instead. If the await Delay(5000); isn't there, cpu load for the thread goes to 50%. I've downloaded your code and I think this might be related to there being multiple PrimS.Telnet.Client classes. I can only instantiate the one in the PrimS.Telnet.40 project.

Issue #2 - Client.IsConnected property doesn't show connection is down when the device I'm checking is physically disconnected after it is initially connected to. So once the connection is disconnected, there is no way to detect that disconnection. According to MSDN for TcpClient.IsConnected:

Because the Connected property only reflects the state of the connection as of the most recent operation, you should attempt to send or receive a message to determine the current state.

For some reason, calling ByteStreamHandler.Read() doesn't force TcpClient to check the connection status. I've been reading that enabling the KeepAlive option in the TcpClient class constructor might fix this problem, this.client.SetSocketOption(System.Net.Sockets.SocketOptionLevel.Socket, System.Net.Sockets.SocketOptionName.KeepAlive, 1); That way I might be able to detect a disconnection and handle it appropriately.

Below is my working console project.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using PrimS.Telnet;
using System.Threading.Tasks;

namespace TelnetTest
{
    class Program
    {
        static void Main(string[] args)
        {
            MainAsync().Wait();
        }

        static async Task MainAsync()
        {
            string ip = "192.168.1.234";
            using (var client = new Client(ip, 23, new System.Threading.CancellationToken()))
            {
                // Wait for login prompt
                var data = await ReadLine(client, TimeSpan.FromSeconds(10));
                if (string.IsNullOrEmpty(data))
                {
                    Console.WriteLine("1 Login Failure to " + ip);
                }
                if (!data.Contains("Your port number is"))
                {
                    // Try one more time
                    data = await ReadLine(client, TimeSpan.FromSeconds(10));
                    if (string.IsNullOrEmpty(data) || !data.Contains("Your port number is"))
                    {
                        Console.WriteLine("2 Login Failure to " + ip);
                    }
                }
                Console.WriteLine("Connected.");
                // Now you just wait for lines to show up.
                while (client.IsConnected)
                {
                    data = await ReadLine(client, TimeSpan.FromSeconds(60));
                    if (!string.IsNullOrEmpty(data))
                    {
                        Console.WriteLine(data);
                    }
                }
                Console.WriteLine("Completed");
            }
        }

        private static async Task<string> ReadLine(Client client, TimeSpan timeout)
        {
            string data = string.Empty;
            var endtime = DateTime.Now.Add(timeout);
            while (!data.Contains("\r\n") && DateTime.Now <= endtime)
            {
                data += client.Read(TimeSpan.FromMilliseconds(1));
                await Delay(5000);
            }
            return data;
        }

        private static Task Delay(double milliseconds)
        {
            var tcs = new TaskCompletionSource<bool>();
            System.Timers.Timer timer = new System.Timers.Timer();
            timer.Elapsed += (obj, args) =>
            {
                tcs.TrySetResult(true);
            };
            timer.Interval = milliseconds;
            timer.AutoReset = false;
            timer.Start();
            return tcs.Task;
        }
    }
}

[9swampy]

Re #1: The reason there are multiple versions of the Client.cs (and a good few others) is because the NuGet package offers roughly equivalent Net 3.5, Net 4.0 and Net 4.5 versions of the library, and you're correct in noting that the async methods are only natively available in the Net 4.5 library. I've not been inclined to add microsoft.bcl.async to the Net 4.0 implementation but if you would like to offer a pull request to add that support I'll consider it.

Re 2#: I think I can see where you're going with this, however there is a mechanism to stop excessive activity in e.g.IsWaitForIncrementalResponse which is called by my implementation of TerminatedReadwhich should be equivalent to your addition of the await Delay in your implementation. However I'm not sure it would be appropriate to include the same in the non-terminated Read you call repeatedly. If you're going to have your own TerminatedRead then I think it's appropriate to require your calling code to make sure it doesn't flood the Read method - as you've done.

[co2chicken]

Thanks for the quick reply.

I'm sorry if I wasn't clear about Issue #2. The problem is that if you only read from the client, the IsConnected property never changes and no exceptions are thrown when the connection is dead. So you can't detect a dead connection and you can't then try to restart it. I ended up fixing this in my code by sending a null byte occasionally, client.Write(new string(new char[] { (char)0 })); This causes an exception to be thrown by TcpClient when the connection is dead.

[9swampy]

No, sorry, you were clear, I had a few drinks in me :-p and fixated on the code. I'll look into the disconnect suggestion.

[co2chicken]

Actually, it seems sending the 'No Opertions' byte (241) is a better way to test that the connection is alive. client.Write(new string(new char[] { (char)241 }));. When I was sending the NUL ((char)0), it was keeping the device from responding; however, I have had no issues with NOP. This has let me reliably detect the dead connection. Alternately, here's another way to check that connection is alive using Socket.Poll() and Socket.Available on the TcpClient.Client property: http://stackoverflow.com/questions/722240/instantly-detect-client-disconnection-from-server-socket

[9swampy]

I've pushed a possible fix 604f077. Can you verify it works for you?

In short I went down the NoOperation write route you first suggested as Socket.Poll() is not wholly reliable according to MSDN. I've adjusted code coverage but haven't gone so far as to set up a lab verification to a real server so would appreciate feedback on whether it works out in your environment.

[co2chicken]

Thanks for the modification.

I ended up just making my own implementation of minimalistic telnet client, tailored to fit my needs. However, I tested your IsConnected property in my code and it has been working for me. I hope that helps.

[9swampy]

Fair enough. Thanks for the confirmation; unfortunately I've been finding the modified branch causes me problems on real world servers so I'm having to look into the implementation again. Glad to have been able to help.