Skip to content
This repository has been archived by the owner on Nov 28, 2018. It is now read-only.

Stomp.on('error') Not Called for TLS Errors #27

Open
joshuarubin opened this issue Jun 20, 2013 · 2 comments
Open

Stomp.on('error') Not Called for TLS Errors #27

joshuarubin opened this issue Jun 20, 2013 · 2 comments
Assignees

Comments

@joshuarubin
Copy link
Contributor

The following code will cause node to terminate on a TLS authentication error:

var stomp = new Stomp(args);
stomp.on('error', function (err) {
  console.error("Stomp Error: " + err);
});
stomp.connect()

I had to add the following (immediately after connect()) to be able to catch it:

stomp.socket.on('error', function (err) {
  console.error("Stomp TLS Error: " + err);
});

The reason for this is due to this code in stomp.js

stomp.socket = tls.connect(stomp.port, stomp.host, stomp.ssl_options, function() {
    log.debug('SSL connection complete');
    if (!stomp.socket.authorized) {
        log.error('SSL is not authorized: '+stomp.socket.authorizationError);
        if (stomp.ssl_validate) {
            _disconnect(stomp);
            return;
        }
    }
    _setupListeners(stomp);
});

_setupListeners(stomp); is only called after tls.connect() succeeds, in the callback.

According to http://nodejs.org/api/tls.html#tls_tls_connect_port_host_options_callback

the callback parameter will be added as a listener for the 'secureConnect' event.

Since there is no socket.on('error') handler when tls.connect fails, stomp never catches the error and never, subsequently, emits its own error.

I did find the above workaround, but it seems that stomp.on('error') should have worked the first time.

@ghost ghost assigned benjaminws Jun 24, 2013
@benjaminws
Copy link
Owner

This code is bad, and I feel bad :(

I've been on vacation, but I'm starting to get back into the swing of things. I'll look into refactoring this code, and hopefully addressing this in a nicer way, as soon as possible.

Thanks for your report, and if you'd like to take a stab at cleaning this up, feel free!

@joshuarubin
Copy link
Contributor Author

Thanks for the update!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants