Keep session links #147
Keep session links #147
Conversation
…hat prevented reattach.
…e reattach/reconnect example to exercise both attach and connect retries.
| var detachPromise = new Promise(function(resolve, reject) { | ||
| var onError = function(err) { reject(err); }; | ||
| self.once(Link.ErrorReceived, onError); | ||
| self.once(Link.Detached, function(info) { | ||
| self.removeListener(Link.ErrorReceived, onError); | ||
| self.session._removeLink(self); |
There was a problem hiding this comment.
something feels wrong about this, shouldn't the session listen to Link.Detach events itself and remove the links from there? I guess this is better stated as: why is Session's internal state something Link cares about?
There was a problem hiding this comment.
I struggled with this. Basically a session should only remove the link when it's given up on itself (either because we called detach or because it's run out of retries). The link is the one who knows when it's given up - and that doesn't coincide with a Detach event. So either the session needs to interrogate the link on whether it might still be trying to attach, or the link needs to tell the session it's done. I chose the latter, but I can see why the former makes sense given our separation of concerns - I can do that.
There was a problem hiding this comment.
Unless I'm reading this incorrectly the code as it stands still removes the link every Link.Detach though right? Nothing different than the existing Link.Detach in Session
There was a problem hiding this comment.
No, it removes it only when the detach comes from our side. When the detach is instigated by the server, it leaves it alone.
…xposing link._shouldReattach as part of the Link's API
| self.emit(AMQPClient.ErrorReceived, e); | ||
| }); | ||
| } | ||
|
|
||
| self._session.on(Session.Mapped, function(s) { |
There was a problem hiding this comment.
I think we need to unregister this listener (existing bug)
I was closer to right than you were - it was more than just not setting session to null.