Socket.io client not working with ackTimeout

[mrabczuk75]

Issue

In V5, setting up a socket.io client with an ackTimeout:

const socket = io(
  'http://api.feathersjs.com',
  {
    ackTimeout: 5000
  }
)
app.configure(socketio(socket))

changes socket.io client's ack callback arguments from:

(response) => { } to (err, response) => { }

which effectively breaks response handling for any service call.

Proposed solution:

Check if socket timeout exists in Service.send() method (transport-commons/client) and handle the args correspondigly:

const socketTimeout = this.connection.flags.timeout || this.connection._opts.ackTimeout
args.push(function (error, ...args) {
   return !socketTimeout
      ? error ? reject(convert(error)) : resolve(args[0])
      : error || args[0] ? reject(convert(error || args[0])) : resolve(args[1])
})

[daffl]

This seems really strange but I could indeed not get the chat working with this setting either. Since this behaviour does not seem to be documented anywhere, it seems like a bug in Socket.io though.

[mrabczuk75]

I’d disagree – it’s related to socket timeout, which is documented here: https://socket.io/docs/v4/client-api/#sockettimeoutvalue

See also example inside https://socket.io/docs/v4/client-api/#socketemitwithackeventname-args

From: David Luecke @.> Sent: Friday, November 17, 2023 10:19 PM To: feathersjs/feathers @.> Cc: mrabczuk75 @.>; Author @.> Subject: Re: [feathersjs/feathers] Socket.io client not working with ackTimeout (Issue #3336)

This seems really strange but I could indeed not get the chat working with this setting either. Since this behaviour does not seem to be documented anywhere, it seems like a bug in Socket.io though.

— Reply to this email directly, view it on GitHub https://github.com/feathersjs/feathers/issues/3336#issuecomment-1817120102 , or unsubscribe https://github.com/notifications/unsubscribe-auth/BD7H5P7WVNPND33TADK4I4TYE7IFXAVCNFSM6AAAAAA7MCYPJKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJXGEZDAMJQGI . You are receiving this because you authored the thread. https://github.com/notifications/beacon/BD7H5P2LJVXTMMNFMPMKDMTYE7IFXA5CNFSM6AAAAAA7MCYPJKWGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTTMJ4GWM.gif Message ID: @. @.> >

[jd1378]

I just got this bug today, and its as mrabczuk75 describes adding ackTimeout breaks the client, removing it brings back the correct behavior

[daffl]

Oh yes, I was able to confirm this but apparently didn't reply here. If we can have tests for the proposed fix it probably makes sense to include it.

[jd1378]

if the ack feature is always used, why not use emitWithAck instead of emit, since emitWithAck seems to be always returning a promise

edit: now I see that emitWithAck does not return all ack values, investigating on socket.io side ... edit 2: it seems socketio uses emit for emitWithAck under the hood anyway

[jd1378]

I'm trying to hack a patch on my code to test it but I can't really find where the magic is happening exactly

I have found these so far but still haven't figure it out: https://github.com/feathersjs/feathers/blob/181fedb026e21f5978cc32cecfc16c176cf78e59/packages/transport-commons/src/client.ts#L11 https://github.com/feathersjs/feathers/blob/181fedb026e21f5978cc32cecfc16c176cf78e59/packages/transport-commons/src/client.ts#L95 https://github.com/feathersjs/feathers/blob/181fedb026e21f5978cc32cecfc16c176cf78e59/packages/socketio-client/src/index.ts#L34

[jd1378]

Okay, tested and made it a bit more clear it works I'll try to make a PR

[jd1378]

I made the PR and added a test, but the test is probably not good since it isn't the actual socket io calling the callback, meaning if it breaks later (behavior change of socket io I mean), we won't know