Exception at vertxEventBusService.removeListener() after reconnect

[leolux]

In the current release 1.1.0 I get the following exception after the vertx server has rebooted:
Error: Parameter handler must be specified
at checkSpecified (index.js:193)
at vertx.EventBus.that.unregisterHandler (index.js:83)
at Object.angular.module.provider.$get.EventBusStub.unregisterHandler (angular-vertxbus.js:223)
at Object.angular.module.provider.$get.util.unregisterHandler (angular-vertxbus.js:547)
at Object.angular.module.provider.$get.wrapped.unregisterHandler (angular-vertxbus.js:664)

Steps to reproduce:

1. start vertx server and client webapp
2. connect the client to the eventbus
3. register a listener and store the unregister-function somewhere
4. reboot the vertx server
5. wait for the client webapp to reconnect / listen to the "connected event"
6. handle that event and remove the privious listener by invoking vertxEventBusService.removeListener(address, unregisterfunction)

The exception occures every time these steps are finished, although the last parameter unregisterfunction is not undefined.

[knalli]

@leolux You are using the the plugin already for some time, I'm remembering. You have no problems with the 1.0.0?

I'll look into it later this day.

[knalli]

Ok. I have added in 4f2d763 an additional timer service for my "IT". I cannot see any problem here. What's the difference?

[knalli]

1. npm run-script install-it-vertx-server downloads and installs a Vert.X locally.
2. npm run-script start-it-vertx-server starts Vert.X on port 8080.
3. npm run-script start-it-web-server starts a web server on port 3000.
4. Open http://localhost:3000/ in your browser.
5. Register the service TimerService
6. Restart 2.

[leolux]

I have tested version 1.0.0. just now. The error picture is almost identical:
Error: Parameter handler must be specified
at checkSpecified (index.js:193)
at that.unregisterHandler (index.js:83)
at Object.EventBusStub.unregisterHandler (angular-vertxbus.js:204)
at Object.util.unregisterHandler (angular-vertxbus.js:528)
at Object.wrapped.unregisterHandler (angular-vertxbus.js:641)

[knalli]

@leolux Well, must be. Because between 1.0.0 and 1.1.0 was not changed anything in that area.

Can you show me the difference to the sample I've given (it) and yours?

[leolux]

Ok. I must setup your example first. I honestly don't know how to run this code/test. I haven't used e2e before.

[leolux]

Do I just have to run the list of commands and steps you have described? I have npm and all that stuff installed already.

[knalli]

See here : #52 (comment)

[leolux]

On canary branch "npm run-script install-it-vertx-server" gives me this error:
cd test/e2e/vertx && ./install.sh

Der Befehl "." ist entweder falsch geschrieben oder
konnte nicht gefunden werden.

npm ERR! Windows_NT 6.3.9600
npm ERR! argv "c:\Program Files\nodejs\node.exe"
" "run-script" "install-it-vertx-server"
npm ERR! node v0.12.2
npm ERR! npm v2.6.1
npm ERR! code ELIFECYCLE
npm ERR! angular-vertxbus@1.1.0 install-it-vertx-se
npm ERR! Exit status 1

[leolux]

I used my git bash to run this command

[knalli]

If you have the Git Bash installed, you can try to run the commands from this (its more unix/posix compatible).

Alternatively you have to reply the commands of the script.

[leolux]

test\e2e\vertx\install.sh: line 2: wget: command not found. I have to install wget first. I'll finish my test tomorrow.

[leolux]

wget seems to have some problems, even when running as administrator:
Kann nicht nach »b11d13e641e8115a8ffeff758626e950382d1a2d@response-content-disposition=attachment;filename=%22vert.x-2.1.1.tar.gz%
...« schreiben (Result too large).

[leolux]

Anyway. The difference to your example is that I remove the previous listener explicitly in step 7 during handling of the event "connected".

[knalli]

Ah, okay. That it is. During reconnect. Thanks.

[knalli]

@leolux 1.1.1 released with the fix. Thank you for hint ;) The issue was already there for some time..

[leolux]

1.1.1.is still producing the same error 2 lines later:
Error: Parameter handler must be specified
at checkSpecified (index.js:193)
at vertx.EventBus.that.unregisterHandler (index.js:83)
at Object.angular.module.provider.$get.EventBusStub.unregisterHandler (angular-vertxbus.js:224)
at Object.angular.module.provider.$get.util.unregisterHandler (angular-vertxbus.js:549)
at Object.angular.module.provider.$get.wrapped.unregisterHandler (angular-vertxbus.js:666)

[leolux]

In line 549 "callbackMap.get(callback)" evaluates to undefined:
vertxEventBus.unregisterHandler(address, callbackMap.get(callback));

The parameter "callback" references the following function:

    function () {
            var index;
            if (unregisterFn) {
              unregisterFn();
            }
            if (wrapped.handlers[address]) {
              index = wrapped.handlers[address].indexOf(callback);
              if (index > -1) {
                wrapped.handlers[address].splice(index, 1);
              }
            }
            if (wrapped.handlers[address].length < 1) {
              wrapped.handlers[address] = void 0;
            }
          }

So "unregisterHandler" should not be invoked, when callbackMap.get(callback)) returns undefined.

[knalli]

@leolux It would be helpful if you can specify the situation more precisely.

Which calls are you invoking exactly? I need a difference between your setup and the provided IT (npm run-script start-it-vertx-server and npm run-script start-it-web-server) which works nicely. Perhaps a look into the client code (

angular-vertxbus/test/e2e/web/app.js

Lines 33 to 43 in 0bbc9c8

	$scope.registerTimeService = function () {
	holder.timeServiceDeconstructor = vertxEventBusService.on('what-time-is-it', function (data) {
	$scope.currentDateTime = new Date(data.time);
	});
	$scope.timeServiceActive = true;
	};
	$scope.deregisterTimeService = function () {
	holder.timeServiceDeconstructor();
	holder.timeServiceDeconstructor = undefined;
	$scope.timeServiceActive = false;
	};

) is helpful too.

[leolux]

I've created a simple reproducer: https://github.com/leolux/angular-vertxbus_removelistener
The relevant code for this issue is here: https://github.com/leolux/angular-vertxbus_removelistener/blob/master/src/main/resources/webroot/js/app.js

[knalli]

Thank you.

Ehm.. perhaps it was not clear enough (it is same pattern as in AJS):

var unregisterfn = null;

function registerBusListener(vertxEventBusService) {
    unregisterfn = vertxEventBusService.on('inbound.test', function(message) {
        console.log('<<<<<<<<<< ', message);
    });
    console.log('Listener registered');
}

function unregisterBusListener(vertxEventBusService) {
    if (typeof unregisterfn === 'function') {
        vertxEventBusService.removeListener('inbound.test', unregisterfn);
        console.log('Listener unregistered');
    }
}

The last piece is wrong. The unregisterfn is the deconstructor.

function unregisterBusListener(vertxEventBusService) {
    if (typeof unregisterfn === 'function') {
        unregisterfn();
        console.log('Listener unregistered');
    }
}

[leolux]

Ok, Thank you!
So if the unregisterfn() removes the listener from the map of handlers what is the method vertxEventBusService.removeListener() good for then?

[knalli]

var callback = function () {};
vertxEventBusService.register('address', callback);
vertxEventBusService.unregister('address', callback);

[leolux]

There are still 2 issues with the unregisterHandler() function after a successfull reconnect. I updated the reproducer and added buttons to register and unregister. The two issues are marked in step 7 and 9.
https://github.com/leolux/angular-vertxbus_removelistener

[knalli]

what do you mean with "The two issues are marked in step 7 and 9" ?

[knalli]

Ah, got it.

[knalli]

Okay, the unregister function is invalid after using. I can add a guard for this prohibiting abuse.

[leolux]

Furthermore I don't understand the concept behind the registerHandler() function. Currently it is possible to register more than one handler for the same address which doesn't make much sens to me. For example a client that reconnects to the server after a server restart receives every message from the server twice because the reconnect itself automatically reconnects the intial handler and the client also registers a handler during the "connected" event.

Maybe we could distinguish between an "connected" event for initialization of the handlers and a new type of event like "reconnected" which tells the client to process further but without adding handlers again.

[leolux]

Okay, the unregister function is invalid after using. I can add a guard for this prohibiting abuse.

This would solve issue 2 (step 9), but would it also solve the first issue in step 7?

[knalli]

Will have to run your project actually to confirm this issue. Later.

Atm I cannot confirm this, because my own tests do not have this issue (reconnect works, but deconstruction also)

[leolux]

Alright, thx.

[knalli]

Furthermore I don't understand the concept behind the registerHandler() function. Currently it is possible to register more than one handler for the same address which doesn't make much sens to me.

Why should a client not be able to listen to the same address twice? The client should not know a global state of active listeners.. ?!

For example a client that reconnects to the server after a server restart receives every message from the server twice because the reconnect itself automatically reconnects the intial handler and the client also registers a handler during the "connected" event.

Well, you said it already. Do not register on reconnect. Register once and be happy.

[leolux]

I agree that a client can handle both connects and reconnects the correct way itself. But the thing is that a user of this adapter must take care of the difference between the initial connect event and the connect event after a reconnect itself because there is no separate event for this. If the user does not distinguish these scenarios, than the client accidentally receives messages twice.

I think it would be more clear for users to have that distinction built into the API. Maybe this could be an enhancement for a later release.

[knalli]

The point is: you can register for an address without being connected. That is the thing using vertxEventBusService.

[leolux]

Why should a client not be able to listen to the same address twice?

I actually don't know if the client receives two messages from the server or only one message which gets distributed to the client listeners internally. If the server would send the message twice than it would waste resources.

[leolux]

The point is: you can register for an address without being connected.

That sounds nice. Didn't know about that yet.

[leolux]

Just to make it clear. Are both of the following ways supposed to do the same job of removing a listener?

var unregisterfn = null;
var callback = function(message) {};
unregisterfn = vertxEventBusService.on('outbound.test', callback) ;
unregisterfn();

var callback = function () {};
vertxEventBusService.register('address', callback);
vertxEventBusService.unregister('address', callback);

[knalli]

Both: yes

[leolux]

I have tested both ways. Currently the first way (using unregisterfn()) contains one or two issues after a reconnect:
https://github.com/leolux/angular-vertxbus_removelistener
And the second way (using a callback function) contains no issues both before and after a reconnect:
https://github.com/leolux/angular-vertxbus_removelistenerByCallback

[knalli]

Please try 1.1.2

[leolux]

I have updated https://github.com/leolux/angular-vertxbus_removelistener to use version 1.1.2. It seems to have fixed the second issue because the error does not occur anymore. Unfortunately the client still receives messages from the server, although the button "Unregister" has been pushed after the reconnect.

[knalli]

Ok, found a bug: #55

New bugs please in a new issue. Thank you!