Skip to content

Order of operations on error/close with connections/channels #20

Open
myndzi opened this Issue Sep 27, 2013 · 16 comments

3 participants

@myndzi
myndzi commented Sep 27, 2013

The docs say that channels implicitly emit close events when a connection closes, but I can't see any code in channel_model.js or channel.js that does this (I assume it's precipitated lower down?)

Either way, say you have a connection with two channels. Is there a defined order that the events will trigger in? If it errors, will I receive three close events and three error events? If it closes, will I receive three close events?

Will the connection emit its event first, or the channels?

Is there a way to tell in the channel close/error event if it was caused by the connection closing? Is there a property to determine the current state of a connection/channel in general?

Sorry if any of these are answered in the docs, I did look first.

@michaelklishin

Channels are closed first in most (if not all) other clients.

@michaelklishin

Some clients (noteably the Java one and all the others that build on top of it) provide you additional information
about the cause of channel or connection closure, including whether it was application or server-initiated.

@myndzi
myndzi commented Sep 27, 2013

So if a channel has an error and I want to reconnect it, can I differentiate between when the channel is closed because of the connection being closed?

@squaremo
Owner

The docs say that channels implicitly emit close events when a connection closes, but I can't see any code in channel_model.js or channel.js that does this (I assume it's precipitated lower down?)

Actually I think that statement in the docs is wrong. I'll fix that.

Either way, say you have a connection with two channels. Is there a defined order that the events will trigger in? If it errors, will I receive three close events and three error events? If it closes, will I receive three close events?
Will the connection emit its event first, or the channels?

The idea is channels close first, then the connection. I do wonder what is most useful for applications though:

  • If an application closes the connection, does it really want all the channels emitting 'close'?
  • If the server closes the connection, is it better to see all the channels close before you see the connection error?

Maybe it's better to only emit 'close' if just that channel has closed (as it is now).

Is there a way to tell in the channel close/error event if it was caused by the connection closing? Is there a property to determine the current state of a connection/channel in general?

Well, at the minute, you'll only see the connection close. But were I to change that, I guess there'd have to be some indication in the event, since you'd see the channels close before the connection. It's pretty ugly, isn't it.

@myndzi
myndzi commented Sep 27, 2013

The current behavior was actually what I was hoping for. I could have tested it out, but I wanted to know if it was actually defined and reliable, if that makes sense.

I imagine any reasonably organized program is going to have channels as subsets somehow of the connection, so I don't see a great need for issuing events for both. Additionally, it makes retry/reconnect behavior simpler if you can rely on the fact that a channel close message means the connection is still open.

Alternatively, if you emit the connection event before the channels, guaranteed, then the programmer can do what they want accurately -- if the channel emits an error, they can set a property or something to indicate the channel is defunct, or manually clean up the channels such that the events won't have any effect, etc.

@myndzi
myndzi commented Sep 28, 2013

Tangentially related: is there a list of potential errors somewhere? I'd like to distinguish between AMQP errors and, say, stupid typos in my source code ;)

Edit: nevermind, forgot about defs.js

@myndzi
myndzi commented Sep 28, 2013

I can't tell if there's a way to access these values from any error handler, but I was thinking more along the lines of a general way to identify errors that are sourced from amqp or the amqp connection vs other errors. Since promises get used, a thrown error in 'my' code gets caught in the library for example, and if I screwed up I want it to crash or act accordingly. On the other hand, an error that causes a channel closure I want to attempt to recover from. I'm not sure how to distinguish the two. I can either check for TypeError, ReferenceError, and whatever else may exist explicitly, or possibly create an error subclass for amqp. I forked the repo, think I'll do the latter.

@squaremo
Owner

@myndzi In general the variety of error emitted isn't very helpful, except insofar as an error invalidates specifically the channel or the connection. Errors in application code (e.g., a consumer callback that throws an exception) should only ever invalidate the channel; connection errors are always protocol breaches or server problems.

That's a good point about promises: at the minute you may not be able to distinguish easily between an RPC failure (trying to check a missing queue) and an application code failure (trying to use an undeclared variable). In only the first case the channel will emit 'error', but it may not have done that by the time the promise is rejected. Ugh. I'll have to think about this.

@squaremo
Owner

The docs say that channels implicitly emit close events when a connection closes, but I can't see any code in channel_model.js or channel.js that does this (I assume it's precipitated lower down?)
Actually I think that statement in the docs is wrong. I'll fix that.

Argh no I am wrong, and the docs are accurate: https://github.com/squaremo/amqp.node/blob/v0.0.2/lib/connection.js#L273

So, channels are implicitly closed when the connection stops (i.e., when it is itself closed). But they won't emit an error.

In summary:

  • RPC fails: pending RPCs on channel rejected, channel emits 'error', 'close'
  • Consumer throws exception: reject RPCs; channel emits 'error' with the exception, 'close'
  • Channel#close(): returned promise resolved, channel emits 'close'
  • Connection#close(): each channel rejects pending RPCs, channel emits 'close', returned promise resolved, connection emits 'close'
  • Connection closed by server: each channel rejects pending RPCs, emits 'close'; connection emits 'error', 'close'

( NB in the case of promises returned from #close, or RPCs, the event is emitted after the promise is resolved, in the code; however, the promise library is likely to schedule the callback using e.g., setImmediate, so it may execute after the event handler. Being close to the metal is complicated eh.)

The tricky one I can see there is know whether a channel has rejected your RPC because e.g., the queue you mentioned doesn't exist, or because the server decided to close the connection: at the point the RPC is rejected you may not have seen the connection error event. Given the difficulty of making things happen in a defined order in Node.JS, the way to distinguish those may be to have different types of error (used both for rejecting promises and for emitting). Usually I don't like dispatching by exception types.

Maybe the thing to do is just to do a crash stop -- if the connection's closed, you're hosed anyway. In that scenario the channels would just be left alone (no RPC rejections, no events emitted).

@squaremo
Owner

Maybe the thing to do is just to do a crash stop

.. by which I mean, emit an 'error' and 'close' from the connection object and leave it at that; not crash the program, although it might result in that.

This was referenced Sep 29, 2013
@squaremo
Owner
squaremo commented Oct 2, 2013

It occurs to me that if the connection issued 'error' immediately on receiving ConnectionClose, that would make all the cases distinguishable.

@myndzi
myndzi commented Oct 2, 2013

Personally, I'd rather have an explicit distinction (Error type, or a property set on the error object, etc.) than an implicit distinction (Connection error emitted first). As you note above, making things happen in a defined order is tricky -- and for that same reason, expecting things to happen in a defined order is not intuitive.

@squaremo
Owner
squaremo commented Oct 2, 2013

@myndzi Yes, fair point. What about the 'crash stop' option?

@myndzi
myndzi commented Oct 2, 2013

I think I realized a good solution. Include the connection in the channel close event. The user can use that object to reestablish the channel, and if the connection is closed, that will result in an error.

This is distinct from doing something like:

self.ch.on('close', function (ch) {
    // some logic
    self.ch = self.conn.createChannel(...);
});

Because in that case, it's possible(?) for 'self.conn' to be reassigned to the new connection before it's referenced in the event. If you supply the connection that the channel was bound to, there is no ambiguity...

(I suppose the channel object must have a connection property somewhere, I could use that to fulfill my need, but making it explicit in this way makes it a lot more obvious)

@squaremo
Owner

I think I realized a good solution. Include the connection in the channel close event.

This seems like a solution that's particular to your code. There is precedent for including a flag in the close event, but even that I am wary of, since it duplicates information provided by emitting 'error'.

Here's the situation on master:

  • RPC fails: channel emits 'error', pending RPCs on channel rejected, channel emits 'close'
  • Consumer throws exception: channel emits 'error' with the exception, rejects RPCs, emits 'close'
  • Channel#close(): returned promise resolved, channel emits 'close'
  • Connection#close(): each channel rejects pending RPCs, channel emits 'close', returned promise resolved, connection emits 'close'
  • Connection closed by server: connection emits 'error'; channels reject pending RPCs and emit 'close'; connection emits 'close'
  • Connection closed by client because it thinks it's detected a protocol error from the server (a case I missed above): connection emits 'error', channels reject pending RPCs and emit 'close'; connection emits 'close'.

For the most part the 'close' events aren't that useful, since they always happen; you might want to log something, perhaps.

More interesting are the error continuations (rejected promises, or callbacks invoked with errors if you use the 'raw' channel interface) and 'error' events. In general you'd would use the error continuations for channel operations, and the error event for connections. In theory you'll know by the time error continuations are invoked whether the connection has been closed, since that always fires first. So, if you care why an operation failed, you can check against some flag you set on('error', ...), or the like.

Thinking about it though, there's no reason the client library oughtn't help out by encoding that hypothetical flag in the errors. Using different error constructors seems like a sound way to do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.