socketio / socket.io-client-swift

Other
5.21k stars 839 forks source link

socket.emitWithAck(...)timingOut(...) timeout callback never called #1462

Closed davidkessler-ch closed 9 months ago

davidkessler-ch commented 10 months ago

We are using socket.emitWithAck and the server has not yet implemented the ack functionality for this event. However, the callback never gets called, i.e. the timingOut never actually times out, independent of the provided duration.

Shouldn't the timeout be called completely independently of if the server even gets an event with that name, let alone bothers about sending an ack?

Interestingly, this still gets logged: LOG OnAckCallback: OnAckCallback for 0 being released

I should also mention: sometimes the app just completely crashed with BAD_ACCESS error at some line that had something to do with acks.. unfortunately I don't remember the exact error / line and neither is it currently happening.. it just sort of "went away" without changing anything.

socket.emitWithAck("someEvent", with: ["someValue"]).timingOut(after: 5) { data in
    // this callback never runs
}

(using version 16.1.0)

logs:


LOG SocketIOClient{/}: Emitting: 20["message",{... some data ...}], Ack: false
LOG SocketEngine: Writing ws: 20["message",{... some data ...}] has data: false
LOG OnAckCallback: OnAckCallback for 0 being released
LOG SocketEngineWebSocket: Sending ws: 20["message",{... some data ...}] as type: 4
received from server: message
LOG SocketEngine: Got message: 42["message",{... some data ...}]
LOG SocketParser: Parsing 2["message",{... some data ...}]
LOG SocketParser: Decoded packet as: SocketPacket {type: 2; data: [message, {
    ... some data ...
}]; id: -1; placeholders: -1; nsp: /}
LOG SocketIOClient{/}: Handling event: message with data: [{
    ... some data ...
}]```
davidkessler-ch commented 9 months ago

@nuclearace

davidkessler-ch commented 9 months ago

Fixed this by removing the weak self capture in the timingOut function. Not sure why self is lost, but retain cycles are not a problem since they are only temporary anyways with asyncAfter(...) { }

Anyways, I could create a pull request for this.