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

Already on GitHub? Sign in to your account

Adapter for reconnect #2

Closed
dignifiedquire opened this Issue Dec 28, 2012 · 16 comments

Comments

Projects
None yet
3 participants
Contributor

dignifiedquire commented Dec 28, 2012

Hey I've tried to use this with reconnect but I couldn't get it to work.

The adapter I've used looked exactly like the default ones except I switched shoe for engine:

var engine = require('engine.io-stream');
var inject = require('reconnect/inject');

var reconnect = inject(function() {
  var args = [].slice.call(arguments);
  return engine.apply(null, args);
});

This worked only so far as if I disconnected it tried to reconnect once but that failed and then it never tried it again. Do you have any idea what's going wrong? I couldn't figure it out..

Owner

Raynos commented Dec 29, 2012

First thing to check for is to see whether the result of engine() is a thing that emits a connect event.

Other then that, I'm not sure what the issue could be.

Contributor

dignifiedquire commented Dec 29, 2012

@Raynos I checked it and the connect event is emitted correctly. Maybe @dominictarr can help out.

it needs to emit 'close', 'error', or 'end' so that reconnect knows the connection dropped.

https://github.com/dominictarr/reconnect/blob/master/inject.js#L35-L37

engine.io-stream emits 'closed' instead....

https://github.com/Raynos/engine.io-stream/blob/master/stream.js#L18

@Raynos what use is a 'closed' event?

or, maybe this is because Raynos is using a streams2 stream for engine.io-stream. If that buffers 'end' until you have called read() then that could cause this problem.

Owner

Raynos commented Dec 30, 2012

@dominictarr queue.end() should emit "end"

Yeah it will buffer the end event, that's the issue.

hmm, so what if the engine.io socket emits 'close' but not 'open'?
reconnect will not pass the stream to the user until 'connect' is emitted, so the user won't get an opportunity to call read().

I don't think that 'end' is the right event to emit here, in this edge-case at least. in net.connect() the stream will emit 'error' if it does not emit 'connect'.

Owner

Raynos commented Dec 30, 2012

@dominictarr so I should emit close as well. Makes sense.

Well, yes.

But I think the main problem here is that this is the arkward case for new streams, where you have an empty stream (ends immediately) that you never call read() on.

Because New Streams assumes you are gonna pipe it eventually, you have to explicitly handle the case where you don't, or it will stay open.

So, I think the solution is to check whether this is an empty stream, and then emit 'close'.

@Raynos is the close event a part of the NewStreams api? I can't remember.

Owner

Raynos commented Dec 30, 2012

@dominictarr it's not.

Doing stream.on("end") and only that is an unhandled edge case I think.

/cc @isaacs

Maybe my stream implementation just doesn't handle it.

Owner

Raynos commented Dec 31, 2012

Fixed.

See reconnect example.

@Raynos Raynos closed this Dec 31, 2012

Contributor

dignifiedquire commented Dec 31, 2012

@Raynos @dominictarr Thanks a lot for your fast help!

Contributor

dignifiedquire commented Dec 31, 2012

I'm afraid the example isn't working.
Cloning it and then

$ npm install
$ make build
$ node server.js

logs endedended... to the page. But after 11 times the server throws a warning

Listening on port 8080
(node) warning: possible EventEmitter memory leak detected. 11 listeners added. Use emitter.setMaxListeners() to increase limit.
Trace
    at WriteStream.EventEmitter.addListener (events.js:175:15)
    at Readable.pipe (/Users/dignifiedquire/opensource/engine.io-stream/node_modules/read-write-stream/node_modules/readable-stream/lib/_stream_readable.js:335:8)
    at EventEmitter.onConnection (/Users/dignifiedquire/opensource/engine.io-stream/examples/reconnect/server.js:34:12)
    at EventEmitter.emit (events.js:96:17)
    at Server.<anonymous> (/Users/dignifiedquire/opensource/engine.io-stream/server.js:40:20)
    at Server.EventEmitter.emit (events.js:96:17)
    at Server.handshake (/Users/dignifiedquire/opensource/engine.io-stream/node_modules/engine.io/lib/server.js:206:8)
    at Server.handleRequest (/Users/dignifiedquire/opensource/engine.io-stream/node_modules/engine.io/lib/server.js:173:10)
    at Server.exports.attach.trns (/Users/dignifiedquire/opensource/engine.io-stream/node_modules/engine.io/lib/engine.io.js:127:14)
    at Server.EventEmitter.emit (events.js:99:17)

Also stopping the server and restarting doesn't end in a reconnect. The page just sits there and does nothing.

Owner

Raynos commented Dec 31, 2012

I'll look into the event emitter leak.

@Raynos Raynos reopened this Jan 7, 2013

Owner

Raynos commented Jan 7, 2013

EventEmitter leak errors are because of isaacs/readable-stream#45

Once that's fixed those will go away.

Owner

Raynos commented Jan 7, 2013

@dignifiedquire I fixed engine.io LearnBoost/engine.io-client#89 to handle XHR polling correctly.

It now works.

@Raynos Raynos closed this Jan 7, 2013

Contributor

dignifiedquire commented Jan 8, 2013

@Raynos Thanks a lot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment