Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Add timeout method to remote socket #4558

Closed

Conversation

walde1024
Copy link
Contributor

The kind of change this PR does introduce

  • a bug fix
  • a new feature
  • an update to the documentation
  • a code change that improves performance
  • other

Current behavior

The RemoteSocket does currently not expose a timeout method. With that I encountered problems when emitting events by using a remote socket that I fetched from another instance by using the redis adapter.

When I emit a message like this:

remoteSocketFromServer2.emit('send', { key: 'value' }, (err, res) => {
    if (err) {
        ...
    } else {
        ...
    }
});

then the client receives the emitted message but the callback of the sender is invoked immediately with a timeout error. Reason for that is that the flags.timeout of the BroadcastOperator is not set when emitting the message.

image

New behavior

Exposes a timeout method that can be called on the RemoteSocket to set a timeout.

Other information (e.g. related issues)

I encountered this problem by using the following dependencies:

{
    "socket.io": "^4.5.4",
    "socket.io-client": "^4.5.4",
    "@socket.io/redis-adapter": "^8.0.0",
}

The following piece of code can be used to reproduce the issue:

import Redis from 'ioredis';
import { createAdapter } from '@socket.io/redis-adapter';

import { Server } from 'socket.io';

import { io as ioClient } from 'socket.io-client';

describe('Redis socket io store System Test', () => {
    let io1: Server;
    let io2: Server;

    beforeAll(async () => {
        io1 = new Server();
        io1.listen(5001);
        setupRedisAdapterForServer(io1);

        io2 = new Server();
        io2.listen(5002);
        setupRedisAdapterForServer(io2);
    });

    function setupRedisAdapterForServer(io: Server) {
        const pubClient = new Redis({
            port: 6379, // Redis port
            host: '127.0.0.1', // Redis host
            username: 'default', // Not used
            password: 'my-top-secret', // Not Used
            db: 0 // Defaults to 0
        });

        const subClient = pubClient.duplicate();
        io.adapter(createAdapter(pubClient, subClient, { requestsTimeout: 5000 }));
    }

    it('scenario', async () => {
        const socket2 = await connectSocket(5002);

        socket2.on('send', (msg, ack) => {
            ack('OK');
        });

        const socketsFromServer2 = await io1.in(socket2.id).fetchSockets();
        const socketFromServer2 = socketsFromServer2[0];

        let result;
        socketFromServer2.emit('send', { key: 'value' }, (err, res) => {
            if (err) {
                result = err;
            } else {
                result = res[0];
            }
        });

        while (!result) {
            await delay(250);
        }

        expect(result).toBe('OK');
    });
});

const delay = (ms) => new Promise((resolve) => setTimeout(resolve, ms));

async function connectSocket(port) {
    const socket = ioClient('http://localhost:' + port, {
        transports: ['websocket']
    });

    const promise = new Promise((resolve, reject) => {
        socket.on('connect', () => {
            resolve('success');
        });
        socket.on('connect_error', (err) => {
            reject(err);
        });
    });

    await promise;

    return socket;
}

@olocwtowns
Copy link

olocwtowns commented Jan 10, 2023

I ran into the same issue today. The bug means you can't actually receive client ack's in all scenarios when using an adapter in clustered servers. I am working around it for now by writing to the operator.flags.timeout property on my RemoteSocket instance 🤢.

@darrachequesne
Copy link
Member

@walde1024 thanks for opening this pull request 👍

The only problem is that local and remote Sockets will not behave similarly, because local Socket will expect one single response:

socket.timeout(1000).emit("hello", (err, res) => {
  console.log(Array.isArray(res)); // false
});

While a remote Socket will return an array with a single response (since it's a broadcast under the hood):

remoteSocket.timeout(1000).emit("hello", (err, res) => {
  console.log(Array.isArray(res)); // true
});

@walde1024
Copy link
Contributor Author

walde1024 commented Jan 14, 2023

@darrachequesne :

Thanks for your reply. My understanding of socket.io and how it works under the hood is very basic but isn't the problem you are mentioning already there? My PR just exposes the timeout method. The handling of the response stays as it is. Right now the caller of the emit method needs to know that he is calling emit on a socket or on a remote socket because:

  1. Calling .timeout on a remote socket will fail
  2. Handling of the result is different

My PR would just solve the first point including the bug that the timeout hits immediately.

@darrachequesne
Copy link
Member

@walde1024 you are right, I was just wondering whether we could elegantly fix both problems.

darrachequesne pushed a commit that referenced this pull request Jan 24, 2023
The RemoteSocket interface, which is returned when the client is
connected on another Socket.IO server of the cluster, was lacking the
`timeout()` method.

Syntax:

```js
const sockets = await io.fetchSockets();

for (const socket of sockets) {
  if (someCondition) {
    socket.timeout(1000).emit("some-event", (err) => {
      if (err) {
        // the client did not acknowledge the event in the given delay
      }
    });
  }
}
```

Related: #4595
@darrachequesne
Copy link
Member

Merged as 0c0eb00. Thanks a lot 👍

@Geczy
Copy link

Geczy commented Jan 28, 2023

thanks for fixing! i came here to report this issue but see its finished

haneenmahd pushed a commit to haneenmahd/socket.io that referenced this pull request Apr 15, 2023
The RemoteSocket interface, which is returned when the client is
connected on another Socket.IO server of the cluster, was lacking the
`timeout()` method.

Syntax:

```js
const sockets = await io.fetchSockets();

for (const socket of sockets) {
  if (someCondition) {
    socket.timeout(1000).emit("some-event", (err) => {
      if (err) {
        // the client did not acknowledge the event in the given delay
      }
    });
  }
}
```

Related: socketio#4595
dzad pushed a commit to dzad/socket.io that referenced this pull request May 29, 2023
The RemoteSocket interface, which is returned when the client is
connected on another Socket.IO server of the cluster, was lacking the
`timeout()` method.

Syntax:

```js
const sockets = await io.fetchSockets();

for (const socket of sockets) {
  if (someCondition) {
    socket.timeout(1000).emit("some-event", (err) => {
      if (err) {
        // the client did not acknowledge the event in the given delay
      }
    });
  }
}
```

Related: socketio#4595
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants