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

Option to disable pendingSend globally #13

Closed
FifineHex opened this issue May 11, 2021 · 14 comments
Closed

Option to disable pendingSend globally #13

FifineHex opened this issue May 11, 2021 · 14 comments

Comments

@FifineHex
Copy link

FifineHex commented May 11, 2021

It would be good having an option to disable _pendingSend

I have a game that uses this library and sometimes occurs of some connections fail and throw the error "WebSocket is not connected and send queue is full" and cause restart at my server, currently for me to deal with this I have to edit the module manually on server and comment on the function "send(data, ignoreMaxQueueSize)"

Pull Request:
#14

@bminer
Copy link
Owner

bminer commented May 12, 2021

You can increase the size of the send queue by setting WebSocketWrapper.MAX_SEND_QUEUE_SIZE. The current size is 10. You could increase to 1000 if needed. You need to be careful how much data you send to a disconnected socket, though. Exceeding the send queue can cause requests to never complete (i.e. reject / resolve for the Promise is never called until it times out) EDIT: this isn't entirely true because requests ignore the MAX_SEND_QUEUE_SIZE.

If the send queue is disabled and you try to send on a disconnected socket, this sounds like it should cause an exception anyway. I could see adding an option to silently ignore sent messages on disconnected sockets, but this sounds a little sketchy also. Thoughts?

@bminer bminer closed this as completed May 12, 2021
@bminer
Copy link
Owner

bminer commented May 12, 2021

Sorry didn't mean to close.

@FifineHex
Copy link
Author

I believe that ignoring seems to be better in this case, this would serve as a second barrier if the software that is calling the module fails in some way to check at that instant that the socket is disconnected, an example is what occurs in my game, there is an exchange of messages between the server and clients, massively including with Broadcast (sends to several of a particular group / room)

In case leaving this as optional, without affecting the other users of the module who may use pendingSend.

Another point to take into consideration, is that even if it increased the limit of the MAX_SEND_QUEUE_SIZE, there would still be a memory expenditure to allocate in the array these requests, depending mainly on the size that are sent in each request or am I wrong?

@bminer
Copy link
Owner

bminer commented May 12, 2021

Is your issue happening on the server side or client side? If on the server side, you may need to listen for socket disconnection, so that you don't write to disconnected sockets. Doing so is an error, I think. If on the client side, it depends on how you want re-connection logic to work. Most users will want to send all messages that got queued up while disconnected. If you instead want to drop these, we can probably modify this lib a bit to make that work.

To answer your question, if you increase MAX_SEND_QUEUE_SIZE, that will cause pending writes to the disconnected socket to use more memory, yes. This only really makes sense on the client side, though because from the server's perspective a given socket will never reconnect. If / when the client reconnects it will look like a completely different socket connection to the server.

Sorry that was a slightly rushed explanation. Does that make sense?

@FifineHex
Copy link
Author

thanks for the reply, yes it is on the server side and there is also a logic to remove from the list when they are disconnected, but for some strange reason even so still keeps some broken connections without sending in disconnect event (there's no auto reconnect in client side)

@bminer
Copy link
Owner

bminer commented May 12, 2021

I think I understand. So, you try to send data to a "broken" socket that subsequently "disconnects?" Then, this exception propagates through the server, ultimately crashing it?

@FifineHex
Copy link
Author

FifineHex commented May 12, 2021

Exactly, I believe that some glitch or momentary connection loss causes this effect.

I use it the same way as in the pull request and don't have these problems anymore.

@bminer
Copy link
Owner

bminer commented May 13, 2021

OK. I think we may just want to add an option to silently ignore send errors. This is not really a send queue issue AFAIK. In other words, instead of send throwing an error, it would silently fail if the option is set.

Would you be willing to share the error stack trace (or a portion of it) here? Is it easy to replicate?

@FifineHex
Copy link
Author

FifineHex commented May 13, 2021

Yes, the problem is not related to queueing, it would be more of an option to disable memory consumption and avoid server restart.

I think it could work by splitting it into two options

  • Deactivate the entire pendingsend system according to a variable (save memory)
  • Skip the throw but still use the pendingsend system (avoid server restart)

Users would have between these two options to better choose what they want


And about reproducing the problem, it is not so simple in tests with few people, it has only occurred when I launch in production when there are more than 70 simultaneous players and have a good amount of time online.

@bminer
Copy link
Owner

bminer commented May 17, 2021

Makes sense. Thanks, @GustavoMorais. I'll implement something and will keep you posted.

@Dussed
Copy link

Dussed commented Jan 24, 2022

Hey - is there any update on a solution to this?
Currently getting hit by this in production myself with similar symptoms.

@FifineHex
Copy link
Author

@Dussed I made a fork where there is this implementation (I use it on my production server)
@p4r4d0x0/ws-server-wrapper
@p4r4d0x0/ws-wrapper

@Dussed
Copy link

Dussed commented Jan 25, 2022

@GustavoMorais thank you very much!

@bminer
Copy link
Owner

bminer commented Jan 25, 2022

@GustavoMorais - After thinking about this a bit more, I don't think there is anything to do here.

You can disable the send queue by setting WebSocketWrapper.MAX_SEND_QUEUE_SIZE to 0.

It's a good idea to catch exceptions when calling the send method on a WebSocketWrapper. send ultimately calls the send function on the underlying WebSocket, which itself can throw exceptions (i.e. if the socket is broken or disconnected). In your PR, you only avoid throwing on send when the socket is disconnected and the queue is full. You can check isConnected before calling send or wrap your send call in a try/catch block to prevent an exception from crashing your server.

Please let me know your thoughts, but I'm going to close this for now.

@bminer bminer closed this as completed Jan 25, 2022
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

3 participants