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

Not receiving all disconnected notifications #38

Open
bridger opened this issue Jul 11, 2019 · 10 comments
Open

Not receiving all disconnected notifications #38

bridger opened this issue Jul 11, 2019 · 10 comments
Milestone

Comments

@bridger
Copy link
Contributor

bridger commented Jul 11, 2019

I'm having trouble cleaning up old connections in my application server. I investigated and saw that it isn't receiving the disconnected callback (defined on WebSocketService.swift) for each connection.

I tried refreshing a browser a lot so there are lots of short lived connections. I got 81 callbacks saying there was a new connection, but only 41 callbacks saying a connection had closed.

These connections probably weren't shut down properly client-side, so the server might not know they are dead. I had a ping handler set up to clean up dead connections, but it looks like that API is only in the non-NIO version of websockets. Can we port over that API? Kitura/Kitura-WebSocket#48

@pushkarnk pushkarnk self-assigned this Jul 11, 2019
@bridger
Copy link
Contributor Author

bridger commented Jul 12, 2019

@pushkarnk On another issue you said

I tested a very simple scenario of handful browser tabs connecting to a ws server and disconnecting. I could see the disconnected(connection:reason) callback being invoked. I'll wait for more details what you observed.

I am seeing disconnected called most of the time, just not every time. The way I measured it was by putting a log on each connect and disconnect. I refreshed the browser a bunch of times and then I counted how many messages for connected and disconnected were in the log.

If you want to share the code for your scenario I can see if I can repro it with your setup, or if it is specific to my application.

Either way, I think the pinging feature would be super useful. It is often the case that a connection dies silently without the client sending a "goingAway" message. The only way I know to detect those ghost connections is by pinging them to test if they are alive.

@pushkarnk
Copy link
Contributor

pushkarnk commented Jul 12, 2019

Sorry, I had posted something related to another issue here. Deleted it. Please ignore the notifications, if any!

@pushkarnk
Copy link
Contributor

@bridger I wasn't able to repro what you said with 10-12 tabs. So, I wrote a JS client that makes N connections to the test WebSocket server, sends two messages and closes one connection per second.

If I make more than 20 connections, I see that the disconnected() callback hasn't been invoked on a handful of them. The number of misses is, of course, random.

You are right. This needs investigation.

@pushkarnk
Copy link
Contributor

I also found a new Kitura-NIO bug testing with the JS client: Kitura/Kitura-NIO#215

@pushkarnk pushkarnk added this to the 2019.14 milestone Jul 15, 2019
@pushkarnk
Copy link
Contributor

Ported Kitura/Kitura-WebSocket#48 through #40

@pushkarnk pushkarnk modified the milestones: 2019.14, 2019.15 Jul 17, 2019
@pushkarnk
Copy link
Contributor

I think I have found the reason for the WebSocket connection not receiving all the disconnected notifications. It is a bug in Kitura-NIO. Here's the possible fix: Kitura/Kitura-NIO#217

I ran a WebSocket client with 500 concurrent connections and closing them one connection per second. We seem to be getting the disconnected callback invoked on all of the connections.

@djones6 djones6 modified the milestones: 2019.15, 2019.16 Jul 31, 2019
@bridger
Copy link
Contributor Author

bridger commented Aug 12, 2019

Today I tried updating everything (Kitura-WebSocket-NIO was on commit b98bb3e and Kitura-NIO was at version 2.2.0). I am unfortunately still seeing some connections never get disconnected. I'm afraid this would cause memory leaks that would crash the server pretty often. If there are any other tests I can run, let me know.

@weissi
Copy link

weissi commented Aug 12, 2019

@bridger to help the Kitura folks debug, could you check if NIO sends a channelInactive event?

@bridger
Copy link
Contributor Author

bridger commented Aug 12, 2019

@weissi helped look into this. It looks like Kitura doesn’t implement the channelInactive or errorCaught callbacks. Without these, we’d only get notified if the channel closes cleanly. If the tcp channel is interrupted then Kitura won’t know about it. The solution is likely implementing both of those callbacks and then notifying about the close.

Sent with GitHawk

@pushkarnk
Copy link
Contributor

Thanks @weissi and @bridger for the investigation - Yes, we should implement those callbacks. I'll take that up (Sorry for the late response, I was away yesterday).

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

5 participants