-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
emitWithAck does not throw on disconnect #4964
Comments
I could indeed reproduce, thanks for the test case. I'm digging into this. Related: socketio/socket.io-client#1546 |
Previously, getting disconnected while waiting for an acknowledgement would create a memory leak, as the acknowledgement was never received and the handler would stay in memory forever. This commit fixes the issue: - handlers that do accept an error as first argument, such as: * `socket.emit("test", (err, value) => { ... })` with `ackTimeout` option * `socket.timeout(5000).emit("test", (err, value) => { ... })` * `const value = await socket.emitWithAck("test")` will now properly resolve with an error and get discarded. - handlers that don't like `socket.emit("test", (value) => { ... });` will simply be discarded upon disconnection Note: the structure of the 'acks' attribute has been left untouched, in order to prevent any breaking change. Related: - #1546 - socketio/socket.io#4964
This should be fixed by socketio/socket.io-client@34cbfbb, included in version 4.7.5. @jd1378 could you please check? |
Hi |
hi @darrachequesne , this line is really necessary? If there is an active ack regardless of whether it has a timeout or not, it should be rejected, correct? because in the current situation the ack's registered by the .emit end up being pending |
@wilywork hi! Yes, I think this is necessary, as the callback does not expect an Error argument: socket.emit("hello", (returnValue) => {
// 'returnValue' would be an Error object, which is unexpected
}); What do you think? |
Describe the bug
Take the following steps:
emitWithAck
to send a message immediatelyemitWithAck
to throw on disconnect, but it does notemitWithAck
still not resolving.To Reproduce
Socket.IO client and server version:
4.7.4
I used the socket.io chat example to make a reproduction repo:
https://github.com/jd1378/socketio-emit-ack-bug
Expected behavior
I expect the
emitWithAck
to throw once the socket is disconnected. OR, I expectemitWithAck
to retry the request, as it's not volatile.Platform:
Additional context
What's even better than the expected behavior? a new option to control the behavior.
what I really want is to be able to detect the failure of my requests and throw, much like how a fetch request would throw if connection fails. and I don't want to retry the request automatically when it's thrown.
The text was updated successfully, but these errors were encountered: