Skip to content

numsub and msgCount are different type #155

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

Merged
merged 1 commit into from
Nov 30, 2016

Conversation

barisusakli
Copy link
Contributor

This was causing a timeout error on .clients and .clientRooms because of this check https://github.com/socketio/socket.io-redis/blob/f2dd9fea22963a697666f135247d6f0de1ee7f29/index.js#L252-Lundefined

request.msgCount was an int and request.numsub was a string.

This was causing a timeout error on `.clients` and `.clientRooms` because of this check https://github.com/socketio/socket.io-redis/blob/f2dd9fea22963a697666f135247d6f0de1ee7f29/index.js#L252-Lundefined

`request.msgCount` was an int and `request.numsub` was a string.
@darrachequesne
Copy link
Member

Are you sure that is the root cause? With:

      console.log(typeof numsub[1]);
      numsub = numsub[1];

When running npm test, it always prints number (node v4). Am I missing something?

And clientRooms shouldn't be impacted, right?

@barisusakli
Copy link
Contributor Author

Here is what I get

console.log(typeof numsub[1]);
string
node v6.9.1
redis server 2.8.12
redis dependency 2.6.3

@barisusakli
Copy link
Contributor Author

I just tested again this only fixes .clients() .clientRooms still times out.

@darrachequesne
Copy link
Member

@barisusakli clientRooms can timeout if the id is unknown (no server will answer the request). Is that your case?

@barisusakli
Copy link
Contributor Author

I am not sure how can I check?

Added a console.log here

  case requestTypes.clientRooms:
        console.log('test', request.timeout);
        clearTimeout(request.timeout);

and it comes up as undefined

@barisusakli
Copy link
Contributor Author

Ignore my previous comment, here is the debug output

29/11 12:36 [20825] - info: NodeBB is now listening on: 0.0.0.0:4568
  socket.io-redis ignore same uid +0ms
  socket.io-redis adding bM1cxwFKoinJ_cBvAAAA to bM1cxwFKoinJ_cBvAAAA  +2s
  socket.io-redis adding bM1cxwFKoinJ_cBvAAAA to uid_1  +4ms
  socket.io-redis adding bM1cxwFKoinJ_cBvAAAA to online_users  +0ms
  socket.io-redis adding bM1cxwFKoinJ_cBvAAAA to sess_jrI40MxmdiALe3qD-ynQgdp6zXxD9j0E  +0ms
  socket.io-redis adding bM1cxwFKoinJ_cBvAAAA to topic_4560  +346ms
  socket.io-redis ignore same uid +201ms
  socket.io-redis ignore same uid +7ms
  socket.io-redis received request {"requestid":"icvbRA","type":1,"sid":"1"} +3s
29/11 12:36 [20825] - error: [plugins] filter:topic.build,  timeout reached while waiting for rooms response
29/11 12:36 [20825] - error: /topic/4560/forking-should-have-2
 Error: timeout reached while waiting for rooms response
    at Timeout._onTimeout (/home/baris/node-forum/node_modules/socket.io-redis/index.js:466:46)
    at ontimeout (timers.js:365:14)
    at tryOnTimeout (timers.js:237:5)
    at Timer.listOnTimeout (timers.js:207:5)
  socket.io-redis removing bM1cxwFKoinJ_cBvAAAA from all rooms +1s
  socket.io-redis removing bM1cxwFKoinJ_cBvAAAA from bM1cxwFKoinJ_cBvAAAA +2ms
  socket.io-redis removing bM1cxwFKoinJ_cBvAAAA from uid_1 +2ms
  socket.io-redis removing bM1cxwFKoinJ_cBvAAAA from online_users +1ms
  socket.io-redis removing bM1cxwFKoinJ_cBvAAAA from sess_jrI40MxmdiALe3qD-ynQgdp6zXxD9j0E +0ms
  socket.io-redis removing bM1cxwFKoinJ_cBvAAAA from topic_4560 +1ms
  socket.io-redis adding Sq_IlhUMALGSg9f4AAAB to Sq_IlhUMALGSg9f4AAAB  +3s
  socket.io-redis adding Sq_IlhUMALGSg9f4AAAB to uid_1  +1ms
  socket.io-redis adding Sq_IlhUMALGSg9f4AAAB to online_users  +0ms
  socket.io-redis adding Sq_IlhUMALGSg9f4AAAB to sess_jrI40MxmdiALe3qD-ynQgdp6zXxD9j0E  +2ms

@darrachequesne
Copy link
Member

For the string numsub, that could be related to https://github.com/antirez/redis/issues/1561.

According to the logs, you request the client id "1", is that normal?

@barisusakli
Copy link
Contributor Author

barisusakli commented Nov 29, 2016

It works when I use a proper sid, but if I remove the change in this PR the .clients call timed out.

 socket.io-redis received request {"requestid":"fOImFo","type":0,"rooms":["topic_4560"]} +8s
  socket.io-redis received response {"requestid":"fOImFo","clients":["Jd7SrLyr4uf05kY0AAAA","gvjKaVkEWL_DpzFOAAAB"]} +3ms
  socket.io-redis removing Jd7SrLyr4uf05kY0AAAA from all rooms +388ms
  socket.io-redis removing Jd7SrLyr4uf05kY0AAAA from Jd7SrLyr4uf05kY0AAAA +2ms
  socket.io-redis removing Jd7SrLyr4uf05kY0AAAA from uid_1 +2ms
  socket.io-redis removing Jd7SrLyr4uf05kY0AAAA from online_users +0ms
  socket.io-redis removing Jd7SrLyr4uf05kY0AAAA from sess_jrI40MxmdiALe3qD-ynQgdp6zXxD9j0E +1ms
  socket.io-redis removing Jd7SrLyr4uf05kY0AAAA from topic_4560 +0ms
  socket.io-redis adding 9A6uLw2vq4QZC4U5AAAC to 9A6uLw2vq4QZC4U5AAAC  +2s
  socket.io-redis adding 9A6uLw2vq4QZC4U5AAAC to uid_1  +1ms
  socket.io-redis adding 9A6uLw2vq4QZC4U5AAAC to online_users  +0ms
  socket.io-redis adding 9A6uLw2vq4QZC4U5AAAC to sess_jrI40MxmdiALe3qD-ynQgdp6zXxD9j0E  +1ms
  socket.io-redis adding 9A6uLw2vq4QZC4U5AAAC to topic_4560  +1s

Here is the code I used,

var io = require.main.require('./src/socket.io').server;

io.in('topic_' + data.templateData.tid).clients(function (err, socketids) {
	console.log(socketids);
	if (err) {
		return callback(err);
	}
	var sid = socketids[0];
	io.of('/').adapter.clientRooms(sid, function (err, rooms) {
		console.log(err, rooms)
	});
});

Not sure if there is a cleaner way of accessing .clientRooms, io.clientRooms is undefined.

And yeah seems like numsub is a string not sure why it is a int for you, maybe version difference in the dependencies.

@darrachequesne darrachequesne merged commit 866aa8f into socketio:master Nov 30, 2016
@darrachequesne darrachequesne added this to the 2.0.1 milestone Dec 8, 2016
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.

2 participants