Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Request for non blocking connection. #5

Closed
fireball87 opened this issue Feb 1, 2016 · 10 comments
Closed

Request for non blocking connection. #5

fireball87 opened this issue Feb 1, 2016 · 10 comments

Comments

@fireball87
Copy link

For the most part everything works as I would think, and very well, but there is no way I can find to do a non blocking connection attempt without creating my own version of TCPClient to be created with an existing System.Net.Sockets connected using TcpClient.ConnectAsync(). As everything else is await-able and non blocking, this is an odd oversight. If I end up having some time I may try to push a modified library that does this as it probably shouldn't be hard looking at the code, but we'll see.

9swampy added a commit that referenced this issue Feb 3, 2016
…nd instead of default OnInitialise maintaining prior behaviour.

#4 Add Write(NoOperation) to check socket hasn't disconnected upon each Connected check.
@9swampy
Copy link
Owner

9swampy commented Feb 3, 2016

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

In short the Client.ctor no longer (optionally to maintain backward compatibility) initialises the connection but instead the connection is created on demand in the existing async Read and Write methods.

@fireball87
Copy link
Author

I assume this will work, the methodology itself works for me, but it'll take a few days to get some time to check it, as this project is for work and I need to test it on their servers, but it's not a job that actually pays me or gives me time to code. I'll have to wait for some slow period soon.

@fireball87
Copy link
Author

I tried this today. What I found is that I was forced to use a constructor with me creating the bytestream. This is fine though perhaps harder then it should be. The issue is, the only constructor to tcpbytestream creates a tcpclient which creates a Sockets.tcpclient using a constructor that automatically connects and blocks while connecting. I'm unsure how I could create a non blocking connection with this as is.

@9swampy
Copy link
Owner

9swampy commented Feb 7, 2016

Any chance you could post the code you had to write and maybe adapt it to show what you would have preferred and I'll try to adjust to accomodate?

@9swampy
Copy link
Owner

9swampy commented Feb 7, 2016

I had more time to look into this this morning and think I've a better solution for you. The following test added in c6bbcdb no longer blocks:

[TestMethod]
public void ShouldNotDelayClientConstruction()
{
  using (DelayedConnectionTelnetServer server = new DelayedConnectionTelnetServer())
  {
    DateTime timeout = DateTime.Now.Add(server.ConnectionDelay);
    using (new Client(server.IPAddress.ToString(), server.Port, new System.Threading.CancellationToken(), ConnectionMode.OnDemand))
    {
      DateTime.Now.Should().BeBefore(timeout, "construction of the delayed server should not block the thread");
    }
  }      
}

@fireball87
Copy link
Author

Looking at this change, my gut is this will still block, it will just block when I await the first read. It still calls the blocking code "this.client = new System.Net.Sockets.TcpClient(hostname, port)" . It passes the test because it doesn't block where you are measuring, but instead moves the blockage outside of the test. The awaitable method of constructing a System.Net.Sockets.TcpClient would be:

System.Net.Sockets.TcpClient newClient = new System.Net.Sockets.TcpClient(); await this.client.ConnectAsync(hostname,port); this.client = newClient;

You would still likely handle this as lazy initialization, though you would need an awaitable and non awaitible version, depending on how and where it was called.

I'll compile and test this now though, just to verify this will work as I expect.

Edit, currently it blocks when I either await TcpClient, or when I try to check if it is connected, whichever comes first. Being able to check if it can connect is useful, and I don't think you should need to have an awaitable version of that. I would think best method would be to duplicate TcpClient, and non-blocking empty creation method Client(), and a ConnectAsync(hostname, port, CancellationToken)

If it helps, I've copied and pasted the code as I am originally using it, up to the points where it blocks. I'll leave off the last sections, because they won't actually add much meaning.
`
public async Task ConnectToBlade(BladeType type, string ipaddress, int port, CancellationToken ctoken, string password, string eqssp)
{

        int timeout = 1000;
        BladeReturnData ret = new BladeReturnData();
        ret.output = "No output set, programmer error?";
        try
        {
            using (PrimS.Telnet.Client client = new PrimS.Telnet.Client(ipaddress, port, ctoken))
            {

                try
                {

                    if (!client.IsConnected)
                    {
                        ret.output = "Timed out connecting to blade.";
                        return ret;

                    }
                    else
                    {

                        string check = await client.TerminatedReadAsync(":", TimeSpan.FromMilliseconds(timeout));

`

@9swampy
Copy link
Owner

9swampy commented Feb 8, 2016

Thanks for the further clarification. Hopefully we're making progress...

It's by design that the await client.TerminatedReadAsync will block until the anticipated string is returned, or timeout, whichever comes first. Is your issue that the timeout won't timeout because the connect blocks instead of calling the connection async and permitting the timeout/token to return before the connect itself times out?

@fireball87
Copy link
Author

The timeout is one thing, but it's actually the minor part. The major part is that if my telnet is on the same thread as something else, say, my gui (it is), or other telnets (later, most likely), it can't work on other code while it is waiting for IO. It consumes the entire thread to process nothing, which is what await is trying to avoid. If I await a delay or your awaitable functions after connection, my gui still functions, and can do other stuff, even as it is waiting on the connection to finish. I also cannot create a list of tasks, and use WhenAll and have it do them as they can, because the connections will consume the entire thread. So while it would normally be able to start the connection on multiples at once, it's stuck waiting for the ones that time out, one by one, at ~30 seconds a piece, freezing my interface in place, and preventing any other work from being done on the thread, waiting on something that uses very minimal system resources.

9swampy added a commit that referenced this issue Feb 9, 2016
… the Client, optionally explicitly triggered by Connect method added. Extract public interface IClient and inherit. Improve code coverage.
@9swampy
Copy link
Owner

9swampy commented Feb 9, 2016

I've pushed an extended connection deferral, but I suspect even this won't take you all the way to resolving the full issue you described. Async/await isn't intended as a replacement for full multi-threaded functionality, but if you have time and offer a pull request that helps you deal with your scenario I'm happy to consider integrating it.

9swampy added a commit that referenced this issue Feb 10, 2016
… the Client, optionally explicitly triggered by Connect method added. Extract public interface IClient and inherit. Improve code coverage.
@fireball87
Copy link
Author

It isn't a replacement for threads no, it is a method of getting more out of your threads, but I fail to see the reason for forcing the "one thread per socket" model when it is unnecessary, and is avoided by the library which you're based on. Spawning and destroying tons of threads just for a few IO blocks seems wasteful. I'm never anywhere close to my thread using enough resources that creating another would actually improve performance. Creating a thread to wait for ~10 IO operations with only minor text handling will have the opposite effect. That said, if that is the methodology you would prefer to force, it's not exactly hard to implement (I've already done it, in fact).

Thanks for working with me, and If I have time I'll try to put something together which allows use of the the non-blocking System.Net.Sockets.TcpClient() creation.

9swampy added a commit that referenced this issue Mar 5, 2016
…nd instead of default OnInitialise maintaining prior behaviour.

#4 Add Write(NoOperation) to check socket hasn't disconnected upon each Connected check.
9swampy added a commit that referenced this issue Mar 5, 2016
… the Client, optionally explicitly triggered by Connect method added. Extract public interface IClient and inherit. Improve code coverage.
9swampy added a commit that referenced this issue Mar 5, 2016
…e. Replicate deferred connection in Net3.5 and Net4.0

Deeper consumption of ConnectAsync but still not full depth: http://stackoverflow.com/questions/9519414/whats-the-difference-between-task-start-wait-and-async-await.
Catch error when Server is disposed before Accepting connection.
Fix WaitFor spinner in Servers to also cancel if not IsListening.
9swampy added a commit that referenced this issue Mar 13, 2016
…nd instead of default OnInitialise maintaining prior behaviour.

#4 Add Write(NoOperation) to check socket hasn't disconnected upon each Connected check.
9swampy added a commit that referenced this issue Mar 13, 2016
… the Client, optionally explicitly triggered by Connect method added. Extract public interface IClient and inherit. Improve code coverage.
@9swampy 9swampy mentioned this issue Aug 31, 2017
@9swampy 9swampy closed this as completed Apr 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants