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

Keep track of used channels and close the connection on stop #69

Merged

Conversation

cjmurph
Copy link
Contributor

@cjmurph cjmurph commented May 9, 2016

TcpListener doesn't keep track of used channels and close them on stop. The result is that any established connection continues to be served even after the listener is stopped.
One example is using Griffin.WebServer.HttpServer to serve a web page. If the HttpServer is stopped, the connection remains, until the client browser is stopped.

@jgauffin
Copy link
Owner

jgauffin commented May 10, 2016

Thank you again :)

A question about the stop method:

Shouldn't you await the channels when they are stopping since the listener.Stop() method is synchronous? Something like:

var tasks = _usedChannels.Values.Select(x => x.CloseAsync()).ToList();
if (tasks.Any())
    Task.WaitAll(tasks);

once the _usedChannels are closed, empty the list in case of the listener being restarted:

_usedChannels.Clear();

@cjmurph
Copy link
Contributor Author

cjmurph commented May 10, 2016

The channels are removed from the dictionary when the channel disconnect event fires. This event fires when you stop the channel so the following is executed.

ITcpChannel removed;
_usedChannels.TryRemove(source.ChannelId, out removed);

In my testing this appears to work well.

@jgauffin
Copy link
Owner

that's true. so that part is OK.

Wait for the channels in Stop() before returning.

@cjmurph
Copy link
Contributor Author

cjmurph commented May 11, 2016

OK, it's your framework, but I think it would be better to not block the stop method. Maybe return and fire a stopped event when the tasks finish, maybe even throw a stopping event first. thoughts?

@jgauffin
Copy link
Owner

If the method had been named InitiateShutdown(), BeginStop() or similar I would have agreed. But the contract says "Stop", thus anyone looking at that method would expect it to completely stop which also includes all channels stopping. I should have done that from the start and I'm glad that you pointed it out.

What you can do is to add the behavior as I suggested to the Stop() method, but also introduce StopAsync() method which would return a Task and internally await all channels. Anyone wanting to just signal stop and then move on can then use that method.

@cjmurph
Copy link
Contributor Author

cjmurph commented May 11, 2016

agreed, how about I block the stop method and if you choose you can merge the pull request and create the asynchronous version to your liking?

@jgauffin
Copy link
Owner

sure

@cjmurph
Copy link
Contributor Author

cjmurph commented May 11, 2016

There you go.
Thanks for the awesome web server by the way! It's a great way to serve up data without needing elevated privileges.

@jgauffin jgauffin merged commit e0e0280 into jgauffin:master May 11, 2016
@jgauffin
Copy link
Owner

thank you.

@jgauffin
Copy link
Owner

I'm working on HTTP2. Will probably be included in Griffin.Framework sometimes during the summer.

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

Successfully merging this pull request may close these issues.

None yet

2 participants