rdavisau / sockets-for-pcl

Cross-platform socket API for Xamarin iOS/Android/Forms, Xamarin.Mac/MonoMac, Windows Phone 8/8.1, Windows Store and Windows Desktop.
MIT License
224 stars 72 forks source link

Null Reference Exception #83

Open vbisbest opened 8 years ago

vbisbest commented 8 years ago

I am getting a null ref exception using ConnectAsync. It seems to happen on this specific server and it happens about 80% of the time. This is on Droid (havent tested others) and on 2.0.0 pre 2.

Code to repro: ` TcpSocketClient socket = new TcpSocketClient(); CancellationTokenSource cts = new CancellationTokenSource();

            cts.CancelAfter(10000);

            var task = socket.ConnectAsync("68.97.208.155",25590, false, cts.Token);`

It breaks on the call and does not return. No stack trace is available. And my own try catch around the function does not get called. Thanks.

vbisbest commented 8 years ago

I am also seeing these reported

System.NullReferenceExceptionObject reference not set to an instance of an object at Sockets.Plugin.Abstractions.NativeExceptionExtensions+<>c.<WrapNativeSocketExceptions>b__1_0 (System.Threading.Tasks.Task t) [0x00042] in <filename unknown>:0 at System.Threading.Tasks.ContinuationResultTaskFromTask 1[TResult].InnerInvoke () [0x00027] in <filename unknown>:0 at System.Threading.Tasks.Task.Execute () [0x00016] in <filename unknown>:0

vbisbest commented 8 years ago

Any luck on this issue?

vbisbest commented 8 years ago

It has something to do with the way exception handling is occuring NativeExceptionExtensions when cancellation token is used. I was able to repro in your source code. Here is the repro code:

http://imgur.com/edit?album_id=IvVgc

And here is the crash http://i.imgur.com/Qu9RZcu.png

rdavisau commented 8 years ago

Hey @vbisbest,

Apologies, I've been away for Evolve and now just on holidays so I haven't kept across the issues. There are two things I think we should try upfront -

  1. Install-Package rda.SocketsForPCL -Version 2.0.0, confirm sure the error still exists (I think it will, but please check this).
  2. Can you share the more of your code from your method? e.g. what is the signature of the method that the code you shared is from, how is it invoked, and what is the signature of the invoking function, when do you await task, where is your try/catch? When you say the code breaks without your try/catch being called, it makes me think there may be an async/await-related problem.

I'll keep an eye out for emails on this issue today so if you're still working on it at the moment let me know the answers to the above and we can keep debugging!

vbisbest commented 8 years ago

Thanks! This is reproduced in the pre code from github. See my screenshot for the full block. You should be able to get the same error with the exact same code. I am working on it right now, so hopefully we can figure this out. Got LOTS of crashes in my app from users.

vbisbest commented 8 years ago

Was that enough to help you reproduce it?

rdavisau commented 8 years ago

Ok - I will not be able to test against the source code until this afternoon, but I have written up a very simple test on android that does (I think) the same thing.

packages.config:

<?xml version="1.0" encoding="utf-8"?>
<packages>
  <package id="rda.SocketsForPCL" version="2.0.0" targetFramework="MonoAndroid44" />
</packages>

MainActivity.cs:

using Android.App;
using Android.Widget;
using Android.OS;
using Sockets.Plugin;
using System.Threading;
using System;

namespace socketsTest
{
    [Activity(Label = "sockets", MainLauncher = true, Icon = "@mipmap/icon")]
    public class MainActivity : Activity
    {
        int count = 1;

        protected override void OnCreate(Bundle savedInstanceState)
        {
            base.OnCreate(savedInstanceState);

            // Set our view from the "main" layout resource
            SetContentView(Resource.Layout.Main);

            // Get our button from the layout resource,
            // and attach an event to it
            Button button = FindViewById<Button>(Resource.Id.myButton);

            button.Click += async (o, e) => {
                button.Text = string.Format("{0} clicks!", count++);

                var sut = new TcpSocketClient();
                var cts = new CancellationTokenSource();
                cts.CancelAfter(TimeSpan.FromSeconds(10));

                var t = sut.ConnectAsync("gommehd.net", 25565, false, cts.Token);

                try { 
                    await t;
                    button.Text = "SUCCESS!!!!";
                }
                catch (Exception ex)
                {
                    button.Text = ex.ToString();
                }
            };
        }
    }
}

With the correct domain, I get 'success'. With the incorrect domain, after 10 seconds, I get an OperationCancelledException, which is appropriate behaviour. I wasn't able to hit the null reference issue by (e.g.) rapidly hitting the button, and the exceptions are all caught, you can set breakpoints and they hit, etc. etc.

I will say that I am unsure about the way the xUnit test runner and/or visual studio are interacting with these tests and I think you are hitting the same weirdness that I hit when trying to use them - one of the reasons that I have not pushed 2.0.0 past pre (ignoring the unlisted version) is because I am not comfortable that I can test it reliably with xunit and I think I want to move it back to my own hand-rolled tests.

So am I missing something with the above? Can you check it over and see whether I am missing something fundamental? Thanks!

vbisbest commented 8 years ago

You are correct, connecting to a good domain is fine. The sample I had in the screenshot was not a valid host or no listener on the port (my app allows users to input their own urls that can be bad or the server may be down).

vbisbest commented 8 years ago

Trying with your code, stand by

vbisbest commented 8 years ago

Still happens with an invalid url. The problem is the exception is never thrown, it just dies inside the socket library. See my screenshot here.

http://imgur.com/IdgDMQ1

That is after the 10 second timeout

rdavisau commented 8 years ago

For me, with your url, in XS, after 10 seconds, I get this:

image

I don't trust that VS dialog - from memory it seemed to break on all 'thrown' exceptions rather than an 'unhandled' exceptions, and you could click continue and maybe see it a couple more times as it bubbled through the code - is that the case?

vbisbest commented 8 years ago

wow, you are right, uggggg. I reset my exception settings to default and now it bubbles properly. Sorry I went down the wrong path. However, I see have the original issue and I cannot reproduce this. And I am not sure where its being thrown as it does not show the top level function. Ideas here?

Xamarin caused by: md52ce486a14f4bcd95899665e9d932190b.JavaProxyThrowable: System.AggregateException: A Task's exception(s) were not observed either by Waiting on the Task or accessing its Exception property. As a result, the unobserved exception was rethrown by the finalizer thread. ---> System.NullReferenceException: Object reference not set to an instance of an object at Sockets.Plugin.Abstractions.NativeExceptionExtensions+<>c.b__1_0 (System.Threading.Tasks.Task t) [0x00042] in :0 at System.Threading.Tasks.ContinuationResultTaskFromTask1[TResult].InnerInvoke () [0x00027] in <filename unknown>:0 at System.Threading.Tasks.Task.Execute () [0x00016] in <filename unknown>:0 --- End of inner exception stack trace --- ---> (Inner Exception #0) System.NullReferenceException: Object reference not set to an instance of an object at Sockets.Plugin.Abstractions.NativeExceptionExtensions+<>c.<WrapNativeSocketExceptions>b__1_0 (System.Threading.Tasks.Task t) [0x00042] in <filename unknown>:0 at System.Threading.Tasks.ContinuationResultTaskFromTask1[TResult].InnerInvoke () [0x00027] in :0 at System.Threading.Tasks.Task.Execute () [0x00016] in :0 <---

rdavisau commented 8 years ago

Don't worry, as my memory is coming back I remember getting very frustrated and confused by that behaviour; for me even resetting the exception settings didn't seem to help, I basically gave up on VS for doing these tests.

As for this other error - I'm pretty sure this is what happens when you don't await a task before it is GC'd. It's possible that this is caused by my code, we would need to look more deeply into it.

As a quick workaround/confirmation, maybe attempting the second option in this answer prevents the teardown?

vbisbest commented 8 years ago

I have it reproducing now, but I cant catch where its happening. My app crashes without breaking even in debug mode.

vbisbest commented 8 years ago

So when I call connectasync again after it times out is when it crashes. Am I not cleaning up properly or should I be wrapping with a using statement? My app tries to connect to many servers at the same time (around 10).

vbisbest commented 8 years ago

Here is the entire block: `private async void ConnectToServer(ServerInfoState2 serverInfoState) { // Connect to the server Debug.WriteLine("Connect to server " + serverInfoState.Server.Address );

        try
        {
            TcpSocketClient socket = new TcpSocketClient();
            CancellationTokenSource cts = new CancellationTokenSource();

            cts.CancelAfter(10000);
            var task = socket.ConnectAsync(serverInfoState.Server.RealIP, serverInfoState.Server.RealPort, false, cts.Token);
            await task;

            if (task.IsFaulted)
            {
                throw task.Exception;
            }

            serverInfoState.Socket = socket;

            Debug.WriteLine("Connection established " + serverInfoState.Server.Address );
            ConnectionEstablished(serverInfoState);
        }
        catch (Exception ex)
        {
            if (ServerInfoError != null)
            {
                serverInfoState.Server.Status = ServerStatus.Error;
                serverInfoState.Server.MOTD = SERVER_ERROR_CANT_REACH;
                ServerInfoError(serverInfoState.Server, ex.Message);
            }
        }
    }

`

rdavisau commented 8 years ago

Can you change your async void signature to async Task (and refactor the caller etc. accordingly)? In my experience async void is generally a recipe for unhappiness where exceptions are involved 💀

vbisbest commented 8 years ago

I am not sure how to refactor this in that way. Do I need to return the task and wait in the caller?

rdavisau commented 8 years ago

Yeah, if you can share some of the caller code including its signature I can help

vbisbest commented 8 years ago

This is brutal, I was able to adjust the code but no difference. I just cant figure it out. Would you be up for a screen share?

rdavisau commented 8 years ago

Yep - I can do in an hour or so?

On 3 May 2016, at 1:04 pm, Ray Kelly notifications@github.com wrote:

This is brutal, I was able to adjust the code but no difference. I just cant figure it out. Would you be up for a screen share?

— You are receiving this because you commented. Reply to this email directly or view it on GitHub

vbisbest commented 8 years ago

perfect!

vbisbest commented 8 years ago

Here is another stack trace with some different info. Xamarin caused by: md52ce486a14f4bcd95899665e9d932190b.JavaProxyThrowable: System.AggregateException: A Task's exception(s) were not observed either by Waiting on the Task or accessing its Exception property. As a result, the unobserved exception was rethrown by the finalizer thread. ---> System.ObjectDisposedException: Cannot access a disposed object. Object name: 'System.Net.Sockets.Socket'. at Sockets.Plugin.Abstractions.NativeExceptionExtensions+<>c.<WrapNativeSocketExceptions>b__1_0 (System.Threading.Tasks.Task t) [0x00027] in C:\Users\rdavis\Source\Repos\sockets-for-pcl-for_real\Sockets\Sockets.Implementation.NET\Extensions\NativeExceptionExtensions.cs:25 at System.Threading.Tasks.ContinuationResultTaskFromTask1[TResult].InnerInvoke () [0x00027] in /Users/builder/data/lanes/3053/a94a03b5/source/mono/external/referencesource/mscorlib/system/threading/Tasks/TaskContinuation.cs:111 at System.Threading.Tasks.Task.Execute () [0x00016] in /Users/builder/data/lanes/3053/a94a03b5/source/mono/external/referencesource/mscorlib/system/threading/Tasks/Task.cs:2523 --- End of inner exception stack trace --- ---> (Inner Exception #0) System.ObjectDisposedException: Cannot access a disposed object. Object name: 'System.Net.Sockets.Socket'. at Sockets.Plugin.Abstractions.NativeExceptionExtensions+<>c.b__1_0 (System.Threading.Tasks.Task t) [0x00027] in C:\Users\rdavis\Source\Repos\sockets-for-pcl-for_real\Sockets\Sockets.Implementation.NET\Extensions\NativeExceptionExtensions.cs:25 at System.Threading.Tasks.ContinuationResultTaskFromTask`1[TResult].InnerInvoke () [0x00027] in /Users/builder/data/lanes/3053/a94a03b5/source/mono/external/referencesource/mscorlib/system/threading/Tasks/TaskContinuation.cs:111 at System.Threading.Tasks.Task.Execute () [0x00016] in /Users/builder/data/lanes/3053/a94a03b5/source/mono/external/referencesource/mscorlib/system/threading/Tasks/Task.cs:2523 <---

at dalvik.system.NativeStart.run(Native Method)`
vbisbest commented 8 years ago

As you know we found that HockeyApp was causing this to happen. Have you been able to work around the issue? Thanks.

rdavisau commented 8 years ago

I've looked into it a little more --

When it sees an UnobservedTaskException, HockeyApp tears down the app (the TraceWriter.WriteTrace has an optional terminate parameter which is true when not specified). This is not expected behaviour in .NET4.5+ (i.e. since async and await were introduced). It is possible for you to opt-in to teardown when a finalised task's exception is not observed using the ThrowUnobservedTaskExceptions configuration item, and this setting is honoured on Xamarin.iOS. I haven't found a way to use app.config settings with Xamarin.Android; if it is possible, the correct behaviour for HockeyApp would be to check that setting and only log an exception/terminate the app if the user had opted in. I'll raise an issue with them. Still, libraries like sockets-for-pcl should not leave any internal task exceptions unobserved, because as you've found, end users can't do anything about them. 🚑

I know you've been working against the source, so I'm going to ask you to insert the below line to TcpSocketClient here (i.e. add this code as the first statement in the block) and check that it resolves the issue.

// ensure we observe the connectTask's exception in case downstream consumers throw on unobserved tasks
connectTask.ContinueWith(t => t.Exception, TaskContinuationOptions.OnlyOnFaulted);

The problem with the current code that the Task.WaitAny that handles the timeout vs connect will only await the first task that completes. In your case, the timeout would complete, at which point sockets-for-pcl disposes the underlying socket. The actual connect task would then fail, because the socket had been disposed. Since the connect task did not end up being awaited by WaitAny, the exception does not affect the running code - we don't care, we cancelled it. But because of that, the exception hasn't been observed, and when the TcpSocketClient is eventually collected, the UnobservedTaskException event is raised, HockeyApp sees it and per the above terminates the app.

Assuming my understanding of things is correct, adding that line will allow you to use HockeyApp again, so let me know how you go!

rdavisau commented 8 years ago

How did this work out? I'm planning to bring it into the code.

vbisbest commented 8 years ago

Haven't been able to try. On a cruise :). But I trust your code. Let it rip!

On May 19, 2016, at 8:38 PM, Ryan Davis notifications@github.com wrote:

How did this work out? I'm planning to bring it into the code.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub

vbisbest commented 8 years ago

Have you been able to create a new build with this change? I am ready to try it out. Thanks!

rdavisau commented 8 years ago

I'll have it in by Sunday evening my time 👍 (edit -- Monday)

rdavisau commented 8 years ago

Here we go - let's see how it goes! :pray:

viccotr commented 6 years ago

Hi, I have the same problem. Already fix signature of my methods from async void to async Task, but still have problem. Use 2.0.2 version. Can somebody help?