Skip to content

Conversation

@anthony-michael-bennett

Setting the option closeTimeout will override the default milliseconds for terminating unresponsive connections.

@anthony-michael-bennett anthony-michael-bennett changed the title Added ability to override of the default closeTimeout. Added ability to override the default closeTimeout. Dec 16, 2025
@anthony-michael-bennett anthony-michael-bennett changed the title Added ability to override the default closeTimeout. Add override for default closeTimeout. Dec 16, 2025
@lpinca
Copy link
Member

lpinca commented Dec 16, 2025

If added the option should also work on the server, but is there a good reason to add it? It is already possible to call websocket.terminate() to force close a connection before the default delay expires. Also, I would prefer to use an env variable to override the default value if the customization is not needed at the connection level.

@anthony-michael-bennett
Copy link
Author

It is possible to call websocket.terminate() but what i want to do is the same thing as closeTimer. I want to leverage the existing code to close sooner than 30 seconds.

I need it for the server to force clients to close when they don't respond. I like it as an option because you could run multiple websocket servers with different needs. One might be more active and need to close clients sooner.

@lpinca
Copy link
Member

lpinca commented Dec 18, 2025

It is possible to call websocket.terminate() but what i want to do is the same thing as closeTimer.

Yes, you can do something like this:

const timer = setTimeout(() => { ws.terminate(); }, 3000);

ws.on('close', () => { clearTimeout(timer); });
ws.close(); 

You also need to run something like https://github.com/websockets/ws?tab=readme-ov-file#how-to-detect-and-close-broken-connections regardless of this change.

At the moment you made the option only work on the client.

@lpinca lpinca force-pushed the feature-option-closeTimeout branch from b5060af to bb60b5b Compare December 18, 2025 09:51
@anthony-michael-bennett
Copy link
Author

Sorry i only had a test on the client. Thank you for the updated test for client and server. That works for me.

@lpinca
Copy link
Member

lpinca commented Dec 18, 2025

Any comment on #2308 (comment)? Does that work for you?

@lpinca
Copy link
Member

lpinca commented Dec 18, 2025

If the thumbs-up means it's enough for you, I'll wait a little while to see if anyone else is interested in this option, otherwise I'll close this PR.

@JerodEwe
Copy link

Thank you for fixing this.
I will share it to the team when merged.

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.

3 participants