Skip to content

Conversation

@paulashfield
Copy link

Add two new Actions - Events and UpdateConfig

Allow for any sequence of numerics and periods in the version number that Asterisk returns (2.5 for Asterisk 13) in the welcome banner

Some extra type checking to prevent exceptions that we have seen in production message flows.

Paul Ashfield added 2 commits November 7, 2014 13:22
Allow for any sequence of numerics and periods in the version number that Asterisk returns (2.5 for Asterisk 13) in the welcome banner
Some extra type checking to prevent exceptions that we have seen in production message flows.
@marcelog
Copy link
Owner

marcelog commented Nov 7, 2014

cool :) can you make sure that the files are 0644 instead of 0755?

@paulashfield
Copy link
Author

Thanks Marcelo, have done that.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm curious about why would this be necessary, since events should be by default an empty array according to: https://github.com/marcelog/Nami/blob/master/src/message/response.js#L42

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was to avoid this exception, in our production logs. I agree it seems unneccesary but here is the trace

/var/www/rte/node_modules/nami/src/nami.js:77
        this.responses[event.actionid].events.push(event);
                                              ^
TypeError: Cannot call method 'push' of undefined
    at Nami.onRawEvent (/var/www/rte/node_modules/nami/src/nami.js:77:47)
    at Nami.EventEmitter.emit (events.js:95:17)
    at Nami.onRawMessage (/var/www/rte/node_modules/nami/src/nami.js:130:14)
    at Nami.EventEmitter.emit (events.js:95:17)
    at Nami.onData (/var/www/rte/node_modules/nami/src/nami.js:153:14)
    at Socket.<anonymous> (/var/www/rte/node_modules/nami/src/nami.js:197:18)
    at Socket.EventEmitter.emit (events.js:95:17)
    at Socket.<anonymous> (_stream_readable.js:736:14)
    at Socket.EventEmitter.emit (events.js:92:17)
    at emitReadable_ (_stream_readable.js:408:10)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay. I think we should dig a bit more in this.. if we got an event associated to a response, we should include it, and not skip it, like the patch does. Makes sense? Could you check what event is exactly that makes this fail? For example, checking if this.responses[event.actionid].events is undefined, and logging the event if so. Perhaps there's another thing going on here

… observed production problem on WANs where sockets being held open following aa ECONNRESET
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