Support acknowledgement timeouts

[juodumas]

Here is a gist that illustrates accessing an existing and a non-existing service over REST and WS: https://gist.github.com/juodumas/8ac8702a0279168cf92f8ff966c5643b

When accessing the non-existing service, feathers-rest throws a Not Found error while feathers-socketio and feathers-primus stay silent.

[ekryski]

You need to add an error handler for errors to be reported properly. You can use the feathers-errors one as is used in a generated app or you can use your own express style middleware. What's happening is you are getting a 404 but because you don't have an error handler in place it's just spitting out the default Express one, which won't get sent over sockets.

[daffl]

We'll have to investigate some more what it does if you try to connect to a non-existent service via Socket.io. I'm actually not sure.

[juodumas]

Both, socket.io and primus are silent: nothing is logged on the server and nothing is sent back to the client when accessing non-existent services. An error handler doesn't help here.

[daffl]

That's what I thought. Unfortunately there does not seem to be a way for the socket libraries to know if you are emitting an unknown event except for maybe overriding the emitter with a catch all or having a meta service in core that returns information about all registered services so we can throw an error when you are trying to get a non-existent service.

[daffl]

One thing we could easily add as a patch release is a timeout for Socket method calls. That way we'd at least get an error if it never returns (for whatever reason).

[juodumas]

The way I see it, currently feathers-socketio, feathers-primus are only usable for incoming real-time events and unusable as the main communications channel. Take a look at this (admittedly contrived) example:

index.js

// npm init --yes
// npm i feathers feathers-socketio feathers-rest
const feathers = require('feathers')
const socketio = require('feathers-socketio')
const rest = require('feathers-rest')

const app = feathers()
        .configure(rest())
        .configure(socketio())
        .use(feathers.static(__dirname))

app.use('test', {
  find(params, cb) {
    process.exit()
  }
})

app.listen(3031)

index.html

<script src="socket.io/socket.io.js"></script>
<script src="//cdn.rawgit.com/feathersjs/feathers-client/v1.1.0/dist/feathers.js"></script>
<script>
  function feathersTransportTest(transport) {
    var app = feathers()

    if (transport == 'rest') app.configure(feathers.rest().fetch(window.fetch.bind(window)))
    else app.configure(feathers.socketio(io()))

    app.service('test').find().then(
      function(r) { console.log(`${transport}:ok`, r) },
      function(r) { console.log(`${transport}:error`, r) }
    )
  }

  feathersTransportTest('rest')
  // feathersTransportTest('socketio')
</script>

When calling app.service('test').find() using feathers-rest, the promise will be rejected via window.fetch with a TypeError: Failed to fetch(…). When calling it with feathers-socketio, there will be no response. So the problem is not only with non-existent services, but the lack of a request-response pattern in general. Since feathers makes it so easy to use WebSockets instead of REST with feathers-client, I feel that you need to take a stance: either state that sockets are only usable for events, or guarantee that the promise returned by a service call is always resolved.

As @daffl points out, adding support for timeouts could be the easiest way to ensure that. But what would be the global default? It would be nice to support timeouts for REST as well as sockets with a global default of e.g. 10 seconds, overridable for each service call, but I am not so sure how it would work with the current signature (http://docs.feathersjs.com/services/readme.html#service-methods). Moreover there would probably be problems with promises, as ES6 promises do not support cancellation and timed out socket/rest calls could still return.

Re. non existent events, there is https://github.com/hden/socketio-wildcard for socket.io and an open bug report: socketio/socket.io#434. Don't know about Primus.

[marshallswain]

The way I see it, currently feathers-socketio, feathers-primus are only usable for incoming real-time events and unusable as the main communications channel.
So the problem is not only with non-existent services, but the lack of a request-response pattern in general.

I'm not sure I'm following correctly. I thought this was a problem with non-existent services. Don't you get a response from a service that does exist?

[ekryski]

@juodumas basically what you are looking for is an "ack" timeout. That's cool and definitely a very valid feature request. 😄

I might reading too much into this but calling it "unusable" is a bit harsh imho. Many message based protocols don't even give you this ability (socket.io, redis, etc.). If you publish stuff to the wrong channel then you made a mistake or you have to implement the ack timeout yourself.

So, I do, and I think I speak for the team when I say, we very much appreciate all the effort you put into this and this is very valid but we have used Feathers socket.io in production quite successfully for a couple years already without the need for this feature so it's definitely not unusable. It definitely could just be improved.

[juodumas]

@ekryski, yes, I am very sorry for phrasing this too harshly. What you as a team are doing with feathers is fantastic. The simple concepts of services, the ease to use multiple transports and backends in feathers is outstanding. And I have just noticed that feathers has been around for a couple of years 👍

Re. sockets, are you actually using them in your apps as the main transport or maybe even without feathers-rest?

[ekryski]

@juodumas for some things, yes we are only using socket.io as the primary transport, especially between services running remotely.

[daffl]

Socket client timeouts are now implemented in the latest provider versions and set to 5000ms by default. They can be configured with app.configure(socketio(socket, { timeout: 2000 })