Open Ruby184 opened 6 years ago
Thanks a lot for this detailed description. I love the idea and yes we can add it to the core.
I don't like the promise version, since it will stop the entire scope of code, if no reply was ever returned from the client. For example:
await socket.send('foo', 'bar')
// all code here is waiting for acknowledgement
I believe, it's fine to accept the 4th argument.
Also I have more concern, how it will work across the cluster? Currently AdonisJs has out of the box cluster support, where messages are broadcasted to all the process and then they send those messages to their respective client.
Never the less, we can start with the basic implementation first and then improve it over time.
No problem. Thank you for your time spent reading all of this.
Yes, you are right, the problem is when promise is waiting indefinitely for the response and it never comes, so there is no way to know, when the emit request fails silently. The solution would be to just reject promise after timeout, when connection is closed or topic is left. But this can be implemented later or as addon package so leave it optional for now.
So i will just add optional ack parameters to emit
methods on both client and server without introducing send
method.
According to cluster. Acknowledgement of events cannot be used when broadcasting. So broadcasting will work without any changes. Event acking is only used when emiting on a single socket connection and passing optional ack callback. Only then the event packet contains the id
property, which signalizes that recieved event should be acknowledged by client / server. I will create a testing repo, check it and share it with you.
I will open a PR as soon as I update the implementation and test it properly.
Cool, I will wait for it. Another question.
If I have got it right, the acknowledgment only works when emitting to a single client and not a group of clients. Also is this the same behaviour with socket.io
?
First sry for not responding for such a long period of time.
I looked into socket.io
implementation. When you use broadcast and try to pass an ack
callback an error is thrown (https://github.com/socketio/socket.io/blob/master/lib/socket.js#L155).
Also one more thing is what we should do when promise gets rejected from one of event callbacks?
socket.on('hello', (name) => {
return 'Hi!'
})
socket.on('hello', (name) => {
throw new Error('rejected')
})
Should we add one more packet type for error ACK_ERROR
with type and message and make ack callbacks node.js style (error first) ? Example:
socket.emit('hello', 'world', (err, data) => {
if (err) {
return console.log('rejected response')
}
console.log(data)
})
Yes, this sounds great and throwing hard exceptions for broadcastToAll
is a nice way to go about it.
@Ruby184 Did you get a chance to work on it?
Sorry, I was busy at work and forget about it. But I can definitely look into it today and open PR.
Also waiting for event acking :)
Hello @Ruby184 did you advance on this feature since september ? I can help you on it if you want it !
Why this feature is required (specific use-cases will be appreciated)?
I would like to use websocket connection to emit event and get the result of it - it is more like request and response flow. This functionality is implemented in socket.io, sails.io, etc. It is a nice feature when you want to get rid of AJAX requests for resource CRUD in SPA and pass all data through websocket. Socket.io implementation example:
Have you tried any other work arounds?
This kind of feature is not easily implemented as addon package, because the core functionality is required on
Socket
andConnection
classes as well as new packet type.Are you willing to work on it with little guidance?
Sure. Just need your approval.
Proposed API
Update current server API as follows.
~~emit
method already has theack
parameter which recieves the error when packet is not sent, so add another callback with nameresponseAck