Skip to content

What is the recommended way to close/dispose an open channel? #271

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

Closed
mbett7 opened this issue Sep 24, 2018 · 9 comments
Closed

What is the recommended way to close/dispose an open channel? #271

mbett7 opened this issue Sep 24, 2018 · 9 comments

Comments

@mbett7
Copy link
Contributor

mbett7 commented Sep 24, 2018

Had a quick look through the docs and tests and didn't see any mention of the proper way to close a IWampChannel.

Reason I ask is that there are two methods IWampChannel.Close() and IWampChannel.Close(string, string), which are a bit ambiguous without checking the comments and implementation.

From what I gather IWampChannel.Close() is like a Dispose() call and just closes the underlying connection, effectively having the same effect as negotiating a GOODBYE with the router.

Whereas, IWampChannel.Close(string, string) negotiates a GOODBYE with the router but leaves the underlying connection open?

So if the IWampChannel isn't going to be re-used is IWampChannel.Close() the recommended approach?

@darkl
Copy link
Member

darkl commented Sep 24, 2018

I looked a bit. It seems that Close just kills the connection (without sending a GOODBYE message), while the other overload sends a GOODBYE message and waits for a GOODBYE message from the router in order to close the connection. There is even some documentation for the parameterless Close method which says Closes the session violently..

I guess I won't ever recommend using the parameterless overload. The session should be closed properly, with specifying a reason and details if possible.

@mbett7
Copy link
Contributor Author

mbett7 commented Sep 24, 2018

Right. Guess I should have tested IWampChannel.Close(string, string) first. Didn't notice that it closes the connection in the background after receiving a GOODBYE message from the router.

As for documentation, I was referring to the documentation site. I didn't see an example or mention of closing an opened channel.

Should IWampChannel.Close(string, string) behave similarly to IWampChannel.Open? i.e. return a task that is completed once Open/Close is finished. Otherwise the caller has to register a handler to the channel.RealmProxy.Monitor.ConnectionBroken to know when the channel is 'Closed'.

@darkl
Copy link
Member

darkl commented Sep 25, 2018

Ok I just pushed some implementation of a Close(string, GoodbyeDetails) overload that returns a task. It is available as v18.9.2-develop on MyGet (NuGet package feed https://www.myget.org/F/wampsharp/api/v3/index.json). Please try it and let me know if works as expected.

Elad

@mbett7
Copy link
Contributor Author

mbett7 commented Sep 25, 2018

Thanks for the quick response!

Unfortunately the close task never seems to complete. After a quick debug it looked like the connection was closing before the client received a GOODBYE response from the router.

i.e. WampSessionClient.RaiseConnectionBroken() was being called before WampSessionClient.Goodbye.

Regardless, would WampSessionClient.RaiseConnectionBroken() be a better place to handle the close task? i.e. either completing or setting an exception in the event a connection breaks?

@darkl
Copy link
Member

darkl commented Sep 25, 2018

This simple example works for me fine, perhaps you can modify it a bit so it breaks?

We call SetTasksErrorsIfNeeded from RaiseConnectionBroken, so the Close task will be set to an exception in case we don't get a GOODBYE reply.

Elad

@mbett7
Copy link
Contributor Author

mbett7 commented Sep 26, 2018

🤦‍♂️ turns out I had a dead lock. Fixed that and the close task completes.

Doesn't quite fix my issue however since the connection is still open when the close task completes.

i.e. this assertion would fail

await channel.Close(WampErrors.CloseNormal, new GoodbyeDetails());
channel.RealmProxy.Monitor.IsConnected.ShouldBe(false);

@darkl
Copy link
Member

darkl commented Sep 26, 2018

Please try 18.9.2.2-develop and see if it works better for you.
Note that sometimes we don't get the GOODBYE response, even if the router sends it, and instead get an exception. I'm not sure why this happens, but it looks like something that is hard to fix.

Elad

@mbett7
Copy link
Contributor Author

mbett7 commented Sep 26, 2018

That did the trick. Thanks!

@darkl
Copy link
Member

darkl commented Sep 26, 2018

This is now available from NuGet gallery in version v18.9.3.

Elad

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants