Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Clearing socket.io send buffer after connection timeout #1532

Closed
TimNZ opened this issue Aug 29, 2019 · 7 comments
Closed

Clearing socket.io send buffer after connection timeout #1532

TimNZ opened this issue Aug 29, 2019 · 7 comments
Labels

Comments

@TimNZ
Copy link
Contributor

TimNZ commented Aug 29, 2019

Tip for people who may not be aware of this behaviour.

Below is my code for a browser client.

An unexpected behaviour I just discovered is after the error hooks are called on a socket disconnect, I had presumed that any messages that had failed to be sent during the timeout window were subsequently discarded.

This is not the case - socket.io adds them to a buffer and will resend when the connection is restored.
This is unwanted behaviour as I have already indicated to the user their action has failed e.g. a #.

Now I clear the socket.io sendBuffer on connect if detected a disconnect previously

Should this be something Feathers supports via an explicit option, or at least document?

https://stackoverflow.com/questions/32131629/socket-io-stop-re-emitting-event-after-x-seconds-first-failed-attempt-to-get-a-r/32261523#32261523

import io from 'socket.io-client';
import feathers from '@feathersjs/client';
import socketio from '@feathersjs/socketio-client';
import authentication from '@feathersjs/authentication-client';
import paramsForServer from 'feathers-hooks-common/lib/services/params-for-server'
import {CookieStorage} from 'cookie-storage'
const socket = io((window.App && window.App.apiUrl) || window.location.origin, {transports: ['websocket'],
      upgrade: false});
const client = feathers();
let socketDisconnectError = false
socket.on('connect',function() {
  if (socketDisconnectError)
  {
    this.sendBuffer = []
    socketDisconnectError = false
  }
})
client.configure(socketio(socket,{timeout: 30000})); 
client.configure(authentication({
  storage: new CookieStorage({path: '/',expires: new Date().addDays(7) }),
  service: 'service/users'
}));
client.paramsForServer = paramsForServer
client.on('server.connection.error',(context) => {
  console.log('server.connection.error',context)
  // TODO: Send notification elsewhere that couldn't connect to the server

})
const ErrorMessageReplaces = [[/Timeout of/i,'Sorry.  We were unable to connect to our server or got no response.  Please try again later.']]

const processTransportErrors = (context) => {
  ErrorMessageReplaces.forEach((pair,i) => {
    if (context.error.message.match(pair[0]))
    {

      context.error.message = pair[1];
      context.app.emit('server.connection.error',context)
      socketDisconnectError = true
      return false;
    }
  })
  return context;

}

client.hooks({
  error: {
    all: [processTransportErrors]
  }
})


export default client;
@daffl
Copy link
Member

daffl commented Sep 3, 2019

I'm wondering if there should be a manual timeout anymore at all. This was originally needed if e.g. you called a service method that didn't exist but with the new Socket format you will get an error right away in that case.

@TimNZ
Copy link
Contributor Author

TimNZ commented Sep 3, 2019

I'm still on Buzzard/v3 so don't know how your v4 arch changes impact on this.

In my scenario, I am debugging/stepping through code on the server,
so a timeout occurs on the client for a service action,
and the message stays in socket.io send buffer queue, never removed, sent once connection has been restored.

@DaddyWarbucks
Copy link
Member

I believe I am experiencing this as well. I have experienced the issue above as well as some odd 401's when paired with authentication.
I first noticed it as I was working on both a front end and a backend that were both running some dev script that auto restarted either on file change. I would randomly get 401's and odd behavior. I also have a mobile app that is currently using the socket transport. I understand the risks in doing so, but I have this app out in beta right now and wanted to see how the socket transport would hold up over mobile connections.

I created a basic repo where this can be reproduced easily: https://github.com/DaddyWarbucks/feathers-auth-test

@DaddyWarbucks
Copy link
Member

DaddyWarbucks commented Oct 9, 2019

I can confirm this is not a bug with feathers or feathers-authentication, as I initially thought. This is the natural behavior of how the socket.io client works. The fix mentioned above by @TimNZ works, or more compactly:

socket.on('reconnect', function() {
    socket.sendBuffer = [];
});

I was receiving authentication errors due to this because the sendBuffer happens outside the control flow of feathers-authentication.

More information can be found here:
https://github.com/socketio/socket.io-client/blob/master/lib/socket.js#L158
https://github.com/socketio/socket.io-client/blob/master/lib/socket.js#L332

@daffl
Copy link
Member

daffl commented Oct 9, 2019

The question would be if that should be included in the clients. I would think so, potentially together with #1537

@daffl daffl added the Client label Oct 11, 2019
@greyivy
Copy link

greyivy commented Oct 18, 2019

It would be helpful to be able to set the timeout option to null as well to disable it completely and use the built-in socket.io functionality.

@daffl
Copy link
Member

daffl commented Mar 18, 2021

This has been closed in the v5 prerelease via #1937

@daffl daffl closed this as completed Mar 18, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants