unosquare / embedio

A tiny, cross-platform, module based web server for .NET
http://unosquare.github.io/embedio
Other
1.46k stars 176 forks source link

EndPointListener not removed from EndPointManager when server shutdown or disposed. #402

Open JRosanowski opened 4 years ago

JRosanowski commented 4 years ago

The EndPointListener is not removed from EndPointManager when the server is shutdown or disposed. We noticed this on iOS since if our app is suspended then iOS will shutdown the listener and make it unusable which leaves you in the position of not being able to restart the server.

It looks like the problem is in EndPointManager.RemovePrefix, it creates a new prefix object and passes it to RemovePrefix

private static void RemovePrefix(string prefix, HttpListener listener)
{
     try
     {
         var lp = new ListenerPrefix(prefix); // <- Creates new prefix object

         if (!lp.IsValid())
             return;

         var epl = GetEpListener(lp.Host, lp.Port, listener, lp.Secure);
         epl.RemovePrefix(lp); // <- Passes new prefix to RemovePrefix
     }
     catch (SocketException)
     {
         // ignored
     }
}

Then in EndPointListener.RemovePrefix it's using the new listener prefix object and it won't be removed because the key of the dictionary is the ListenerPrefix reference and the new ListenerPrefix object won't match any object in the dictionary.

public void RemovePrefix(ListenerPrefix prefix)
{
    // snip....

    Dictionary<ListenerPrefix, HttpListener> prefs, p2;

    do
    {
        prefs = _prefixes;
        if (!prefs.ContainsKey(prefix))
            break; // <- exits here since the new ListenerPrefix object doesn't match any dictionary object

        p2 = prefs.ToDictionary(x => x.Key, x => x.Value);
        p2.Remove(prefix);
    }
    while (Interlocked.CompareExchange(ref _prefixes, p2, prefs) != prefs);
    CheckIfRemove();
}

Thanks

geoperez commented 4 years ago

The last weekend I was checking issue #332, and I think this is related.

geoperez commented 4 years ago

I was testing this issue, but I was not able to reproduce the issue:

private static void Main(string[] args)
        {
            var url = args.Length > 0 ? args[0] : "http://*:8877";

            using (var ctSource = new CancellationTokenSource())
            {
                var web1 = new WebServer(url);

                web1.RunAsync(ctSource.Token);

                Task.Delay(100).Wait();

                ctSource.Cancel();

                web1.State.ToString().Info();

                var web2 = new WebServer(url);
                web2.OnAny(async ctx => await ctx.SendStringAsync("HOLA", "text/plain", Encoding.UTF8));

                web2.RunAsync();

                Console.ReadLine();
            }

I tested calling the URL and the correct response was sent. How are you disposing of the previous webserver?

JRosanowski commented 4 years ago

Are you testing on iOS? On other platforms the previous EndPointListener is still usable so you end up with it passed back and you can reuse it. On iOS the previous one is no longer usable.

In any case the more obvious issue is that when RemovePrefix is called it's passed a new object for the ListenerPrefix and _prefixes are using ListenerPrefix references as the key. You won't find a prefix reference key that is the same as the new ListenerPrefix reference that has just been created.

geoperez commented 4 years ago

I don't have a Mac computer to test with iOS. Anyway, as you mentioned, there is an obvious issue. I'll try to change the logic.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

geoperez commented 4 years ago

@rdeago should we check this for v4?

rdeago commented 4 years ago

It would be great! We still wouldn't be addressing the elephant in the room, but a bug in endpoint lifetime management is always good to solve.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

rdeago commented 4 years ago

Reopening. @geoperez is there a label or something to tell the bot to not close an issue?

geoperez commented 4 years ago

You can find the stale.yml in the .github folder. By default the values are:

exemptLabels:
  - pinned
  - security

Let me know if you need to exclude another one!

rdeago commented 4 years ago

We don't even have those labels.

Just replace "security" with "area:security". I'll add the "pinned" label, and pin every open issue on the 4.0.0 milestone.