Vert.x 2.1.3 Does unregisterHandler call the callback?

[bytor99999]

Here is some new code I wrote for our timers. To handle timers that are started on one node, but another node needs to cancel that timer, I set up handlers when I started the timer. Registered it to "nodeID":timerID address. So I also try to unregister the handler if a message is received at this address after it cancels the timer. Here is the code

String timerKey = getTimerKey(timerID)
log.trace("Register handler for gameTimer=${timerKey}")
vertx.registerHandler(timerKey, { m ->
  log.trace("Canceling timer")
  vertx.cancelTimer(timerID)
  pokerGame.getTimersSetForGame().remove(gameTimerKey)
  vertx.unregisterHandler(timerKey, { unregisterDone ->
    log.trace("Unregister handler from cancel for gameTimer=${timerKey}")
  })
})

But I never see the last log in my console. I see the first two above it, but not the one in the callback. 

Thanks

Mark

[Tim Fox]

Mark,

Do you have some actual code I can run?

There are no methods "registerHandler" or "unregisterHandler" on a Vertx object.

--
You received this message because you are subscribed to the Google Groups "vert.x" group.
To unsubscribe from this group and stop receiving emails from it, send an email to vertx+un...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[bytor99999]

Sorry we have a Wrapper class.

In the wrapper class we have...

void unregisterHandler(String address, Closure handler) {
  eventBus.unregisterHandler(address, handler)
}

void unregisterHandler(String address, Closure handler, Closure resultHandler) {
  eventBus.unregisterHandler(address, handler, resultHandler)
}

public void registerForEvents(String name, GameEvent event, Closure handler) {
  log.trace("'${name}' registering for events of type '${event}' on address '${event.fqen}'")
  eventBus.registerHandler(event.fqen, handler)
}

void registerHandler(String address, Closure handler, Closure resultHandler) {
  eventBus.registerHandler(address, handler, resultHandler)
}

void registerHandler(String address, Closure handler) {
  eventBus.registerHandler(address, handler)
}

[bytor99999]

But looking at it I think I need to call the one that takes two handlers. Although what is the first handler do? I want the resultHandler as the closure.

Mark

[Tim Fox]

On 01/05/15 22:04, bytor99999 wrote:

But looking at it I think I need to call the one that takes two handlers. Although what is the first handler do?

The first handler is the handler you want to unregister. The second handler is the handler that you want to be called once the handler has been unregistered.

I want the resultHandler as the closure.

Mark

On Friday, May 1, 2015 at 2:03:10 PM UTC-7, bytor99999 wrote:

Sorry we have a Wrapper class.

In the wrapper class we have...

void unregisterHandler(String address, Closure handler) {
  eventBus.unregisterHandler(address, handler)
}

void unregisterHandler(String address, Closure handler, Closure resultHandler) {
  eventBus.unregisterHandler(address, handler, resultHandler)
}

public void registerForEvents(String name, GameEvent event, Closure handler) {
  log.trace("'${name}' registering for events of type '${event}' on address '${event.fqen}'")
  eventBus.registerHandler(event.fqen, handler)
}

void registerHandler(String address, Closure handler, Closure resultHandler) {
  eventBus.registerHandler(address, handler, resultHandler)
}

void registerHandler(String address, Closure handler) {
  eventBus.registerHandler(address, handler)
}

[bytor99999]

So why the first? Isn't the Handler going to be unregistered via the Address in the first parameter? Anyway, I don't think my handlers are being unregistered. Let me post the whole method and maybe you can see what I am missing. One note, is my re-write simplification of code so far, I haven't yet seen a missed timer fire.

Thanks

private String startTimerInternalGame(PokerGame pokerGame, String gameTimerKey, Long timeout, String method, Map<String, Object> args) {
  String tableID = pokerGame.getTableID()
  Long timerID = vertxWrapper.setTimer(timeout, { Long timerID ->
    final String timerKey = getTimerKey(timerID)
    try {
      gameStateManager.doGameMutationWithLock(new BasicGameStateMutation(tableID, method, args))
      pokerGame.getTimersSetForGame().remove(gameTimerKey)
      log.trace("Trying to unregister handler for gameTimer=$timerKey")
      vertxWrapper.unregisterHandler(timerKey, {nothing ->}, { unregisterDone ->
        log.trace("Unregister handler after firing for gameTimer=$timerKey")
      })
      pokerGame.firedTimers.put(gameTimerKey, timerKey)
    } catch (Throwable e) {
      log.error("Timer mutation failed for gameTimer=$timerKey", e)
    }
  })

  String timerKey = getTimerKey(timerID)
  log.trace("Register handler for gameTimer=${timerKey}")

  vertxWrapper.registerHandler(timerKey, { m ->
    log.trace("Canceling timer")
    vertxWrapper.cancelTimer(timerID)
    pokerGame.getTimersSetForGame().remove(gameTimerKey)
    vertxWrapper.unregisterHandler(timerKey, {nothing ->}, { unregisterDone ->

      log.trace("Unregister handler from cancel for gameTimer=${timerKey}")
    })
  })

  return timerKey
}

Mark

[Tim Fox]

On 02/05/15 02:01, bytor99999 wrote:

So why the first? Isn't the Handler going to be unregistered via the Address in the first parameter?

In Vert.x 2.x you need both the address AND the handler in order to unregister it - this is because an address can have many handlers registered with it, so just supplying the address is not enough information.

http://vertx.io/core_manual_groovy.html#registering-and-unregistering-handlers

So when you unregister you must use the same address and the *exact same handler* to unregister it as you used to register it.

Anyway, I don't think my handlers are being unregistered. Let me post the whole method and maybe you can see what I am missing. One note, is my re-write simplification of code so far, I haven't yet seen a missed timer fire.

Thanks

private String startTimerInternalGame(PokerGame pokerGame, String gameTimerKey, Long timeout, String method, Map<String, Object> args) {
  String tableID = pokerGame.getTableID()
  Long timerID = vertxWrapper.setTimer(timeout, { Long timerID ->
    final String timerKey = getTimerKey(timerID)
    try {

What is the following method? The name worries me a bit. How long does it take to execute? Is it blocking? withLock implies there is some kind of global lock - will this scale. The fact that this is in a timer handler and you say you have a million or more timers worries me...

      gameStateManager.doGameMutationWithLock(new BasicGameStateMutation(tableID, method, args))

      pokerGame.getTimersSetForGame().remove(gameTimerKey)
      log.trace("Trying to unregister handler for gameTimer=$timerKey")
      vertxWrapper.unregisterHandler(timerKey, {nothing ->},

This won't unregister the handler. You need to pass in the handler too.

 { unregisterDone ->
        log.trace("Unregister handler after firing for gameTimer=$timerKey")
      })
      pokerGame.firedTimers.put(gameTimerKey, timerKey)
    } catch (Throwable e) {
      log.error("Timer mutation failed for gameTimer=$timerKey", e)
    }
  })

  String timerKey = getTimerKey(timerID)
  log.trace("Register handler for gameTimer=${timerKey}")
  vertxWrapper.registerHandler(timerKey, { m ->
    log.trace("Canceling timer")
    vertxWrapper.cancelTimer(timerID)
    pokerGame.getTimersSetForGame().remove(gameTimerKey)
    vertxWrapper.unregisterHandler(timerKey, {nothing ->}, { unregisterDone ->
      log.trace("Unregister handler from cancel for gameTimer=${timerKey}")
    })
  })
  return timerKey
}

Mark

[bytor99999]

In Vert.x 2.x you need both the address AND the handler in order to unregister it - this is because an address can have many handlers registered with it, so just supplying the address is not enough information.

http://vertx.io/core_manual_groovy.html#registering-and-unregistering-handlers

So when you unregister you must use the same address and the *exact same handler* to unregister it as you used to register it.

I was afraid of that because I don't have the handler defined till after the timer has been set because the address of the handler has to include the timerID which isn't set till the timer is started. And when the timer fires I need unregister the hander inside the timer firing handler, which will have the timerID but hasn't registered the handler yet for that address. Cart before the horse.

 

What is the following method? The name worries me a bit. How long does it take to execute? Is it blocking? withLock implies there is some kind of global lock - will this scale. The fact that this is in a timer handler and you say you have a million or more timers worries me...

      gameStateManager.doGameMutationWithLock(new BasicGameStateMutation(tableID, method, args))

This is the code to actually do something in the game. It is a State Machine, and basically just calling a method on the PokerTable class to update its state. The code that is running inside the lock might take 1 ms at most. We also have many Threads in the thread pool. We have had at least 4000 bots playing on tables for a few hours with this code (The state machine and Locks wasn't my idea, I have to add)

But whether it is locking there or in a database, it is still needed to have that particular game/table locked till the mutation is done. Here is the locking code, so you can see what we do.

boolean gotLock = false;
try {
  if (HazelcastUtil.getLocksMap().tryLock(gameID, LOCK_TIMEOUT, TimeUnit.SECONDS)) {
    gotLock = true;
    PokerGame game = getGame(gameID);
    gameStateMutation.game = game;
    gameStateMutation.mutate();
    List<Message> messages = game.getMessages();
    game.clearMessages();
    saveGame(game);
    vertxWrapper.dispatchAll(messages)
    return messages;
  } else {
    throw new RuntimeException("Could not get lock for game " + gameID + " in " + LOCK_TIMEOUT + " seconds");
  }
} catch (Exception e) {
  log.error("Game state mutation error: ", e)
  throw new RuntimeException("Game state mutation failed gameID=$gameID", e);
} finally {
  if (gotLock) {
    HazelcastUtil.getLocksMap().forceUnlock(gameID);
    HazelcastUtil.getLocksMap().remove(gameID);
    /*Object[] args = [swatch.getTime(), gameStateMutation.gameID, method];
    if (swatch.getTime() > 1000) {
      log.warn("Took {}ms to finish game state mutation for table {} and method {}", args);
    }*/
  }
}

Thanks

Mark

[Tim Fox]

On 02/05/15 17:13, bytor99999 wrote:

In Vert.x 2.x you need both the address AND the handler in order to unregister it - this is because an address can have many handlers registered with it, so just supplying the address is not enough information.

http://vertx.io/core_manual_groovy.html#registering-and-unregistering-handlers

So when you unregister you must use the same address and the *exact same handler* to unregister it as you used to register it.

I was afraid of that because I don't have the handler defined till after the timer has been set because the address of the handler has to include the timerID which isn't set till the timer is started. And when the timer fires I need unregister the hander inside the timer firing handler, which will have the timerID but hasn't registered the handler yet for that address. Cart before the horse.

This should be easily fixable by moving factoring stuff out into different methods.

 

What is the following method? The name worries me a bit. How long does it take to execute? Is it blocking? withLock implies there is some kind of global lock - will this scale. The fact that this is in a timer handler and you say you have a million or more timers worries me...

      gameStateManager.doGameMutationWithLock(new BasicGameStateMutation(tableID, method, args))

This is the code to actually do something in the game. It is a State Machine, and basically just calling a method on the PokerTable class to update its state. The code that is running inside the lock might take 1 ms at most.

I'd doubt that. Looks like you're doing a Hazelcast tryLock operation, which is likely to require a network round trip.

Do you have one state machine per game?

Also.. how does

PokerGame game = getGame(gameID);

Actually get the game?

I presume this can run on any node of the cluster, or why else would you use a Hazelcast lock, so presumably the Game state might be on another node too? So maybe another network round trip to get that...

I'd suggest that this architectural approach is quite un-Vert.x-y and will scale poorly. A more Vert.x-y approach would be to model each game as a verticle instance and then you can interact with that game (e.g. mutate its state) by sending it messages. You wouldn't need any locking as verticles are inherently single threaded, and you wouldn't need worker verticles. You also wouldn't need to maintain game state somewhere and retrieve it, since the state would be in the verticle. This is more of an actor-like approach to the problem.

Just to do a little maths here - let's say the code in the timer, takes on average 5 ms to execute.

If you have 4 million timers in 10 minutes that means, in total it's going to take 20 million ms in total to execute, every 10 minutes.

20 million ms = 333 minutes! So you'll need to cram in 333 minutes of CPU processing into 10 minutes of wall clock time - well that's clearly impossible on a single thread (without breaking the laws of physics) so you'll need at least 34 cores just to stay on top of it, and that's assuming all the work is perfectly parallelisable, which it's unlikely to be because you're using hazelcast to manage locks...

Even if processing does take 1ms as you say, you still have a big issue. It seems quite likely to me based on your description this could be the source of your timer issues. I strongly suspect they are backing up because you just can't process them in time thus appearing to get "lost".

Of course, I could be completely wrong as I haven't seen your actual system, but making a judgement based on what I know so far, I'd say it's a strong possibility.

We also have many Threads in the thread pool. We have had at least 4000 bots playing on tables for a few hours with this code (The state machine and Locks wasn't my idea, I have to add)

But whether it is locking there or in a database, it is still needed to have that particular game/table locked till the mutation is done. Here is the locking code, so you can see what we do.

boolean gotLock = false;
try {
  if (HazelcastUtil.getLocksMap().tryLock(gameID, LOCK_TIMEOUT, TimeUnit.SECONDS)) {
    gotLock = true;
    PokerGame game = getGame(gameID);
    gameStateMutation.game = game;
    gameStateMutation.mutate();
    List<Message> messages = game.getMessages();
    game.clearMessages();
    saveGame(game);
    vertxWrapper.dispatchAll(messages)
    return messages;
  } else {
    throw new RuntimeException("Could not get lock for game " + gameID + 

" in "<
/span> + LOCK_TIMEOUT + " seconds");

  }
} catch (Exception e) {
  log.error("Game state mutation error: ", e)
  throw new RuntimeException("Game state mutation failed gameID=$gameID", e);
} finally {
  if (gotLock) {
    HazelcastUtil.getLocksMap().forceUnlock(gameID);
    HazelcastUtil.getLocksMap().remove(gameID);
    /*Object[] args = [swatch.getTime(), gameStateMutation.gameID, method];
    if (swatch.getTime() > 1000) {
      log.warn("Took {}ms to finish game state mutation for table {} and method {}", args);
    }*/
  }
}

Thanks

Mark

[Tim Fox]

On 02/05/15 17:44, Tim Fox wrote:

On 02/05/15 17:13, bytor99999 wrote:

In Vert.x 2.x you need both the address AND the handler in order to unregister it - this is because an address can have many handlers registered with it, so just supplying the address is not enough information.

http://vertx.io/core_manual_groovy.html#registering-and-unregistering-handlers

So when you unregister you must use the same address and the *exact same handler* to unregister it as you used to register it.

I was afraid of that because I don't have the handler defined till after the timer has been set because the address of the handler has to include the timerID which isn't set till the timer is started. And when the timer fires I need unregister the hander inside the timer firing handler, which will have the timerID but hasn't registered the handler yet for that address. Cart before the horse.

This should be easily fixable by moving factoring stuff out into different methods.

 

What is the following method? The name worries me a bit. How long does it take to execute? Is it blocking? withLock implies there is some kind of global lock - will this scale. The fact that this is in a timer handler and you say you have a million or more timers worries me...

      gameStateManager.doGameMutationWithLock(new BasicGameStateMutation(tableID, method, args))

This is the code to actually do something in the game. It is a State Machine, and basically just calling a method on the PokerTable class to update its state. The code that is running inside the lock might take 1 ms at most.

I'd doubt that. Looks like you're doing a Hazelcast tryLock operation, which is likely to require a network round trip.

Do you have one state machine per game?

Also.. how does

PokerGame game = getGame(gameID);

Actually get the game?

I presume this can run on any node of the cluster, or why else would you use a Hazelcast lock, so presumably the Game state might be on another node too? So maybe another network round trip to get that...

I'd suggest that this architectural approach is quite un-Vert.x-y and will scale poorly. A more Vert.x-y approach would be to model each game as a verticle instance and then you can interact with that game (e.g. mutate its state) by sending it messages. You wouldn't need any locking as verticles are inherently single threaded, and you wouldn't need worker verticles. You also wouldn't need to maintain game state somewhere and retrieve it, since the state would be in the verticle. This is more of an actor-like approach to the problem.

Just to do a little maths here - let's say the code in the timer, takes on average 5 ms to execute.

If you have 4 million timers in 10 minutes that means, in total it's going to take 20 million ms in total to execute, every 10 minutes.

20 million ms = 333 minutes! So you'll need to cram in 333 minutes of CPU processing

Actually it wouldn't (all) be processing time, as it's performing blocking network operations so probably much of it is wait time, which probably accounts for your observation that CPU load is actually low.

into 10 minutes of wall clock time - well that's clearly impossible on a single thread (without breaking the laws of physics) so you'll need at least 34 cores just to stay on top of it, and that's assuming all the work is perfectly parallelisable, which it's unlikely to be because you're using hazelcast to manage locks...

Even if processing does take 1ms as you say, you still have a big issue. It seems quite likely to me based on your description this could be the source of your timer issues. I strongly suspect they are backing up because you just can't process them in time thus appearing to get "lost".

Of course, I could be completely wrong as I haven't seen your actual system, but making a judgement based on what I know so far, I'd say it's a strong possibility.

We also have many Threads in the thread pool. We have had at least 4000 bots playing on tables for a few hours with this code (The state machine and Locks wasn't my idea, I have to add)

But whether it is locking there or in a database, it is still needed to have that particular game/table locked till the mutation is done. Here is the locking code, so you can see what we do.

boolean gotLock = false;
try {
  if (HazelcastUtil.getLocksMap().tryLock(gameID, LOCK_TIMEOUT, TimeUnit.SECONDS)) {
    gotLock = true;
    PokerGame game = getGame(gameID);
    gameStateMutation.game = game;
    gameStateMutation.mutate();
    List<Message> messages = game.getMessages();
    game.clearMessages();
    saveGame(game);
    vertxWrapper.dispatchAll(messages)
    return messages;
  } else {

    throw new RuntimeException("Could not get lock for game " + gameID + " in "&
lt;
/span> + LOCK_TIMEOUT + " seconds");

  }
} catch (Exception e) {
  log.error("Game state mutation error: ", e)
  throw new RuntimeException("Game state mutation failed gameID=$gameID", e);
} finally {
  if (gotLock) {
    HazelcastUtil.getLocksMap().forceUnlock(gameID);
    HazelcastUtil.getLocksMap().remove(gameID);
    /*Object[] args = [swatch.getTime(), gameStateMutation.gameID, method];
    if (swatch.getTime() > 1000) {
      log.warn("Took {}ms to finish game state mutation for table {} and method {}", args);
    }*/
  }
}

Thanks

Mark

[bytor99999]

Thanks Tim. Unfortunately, since our launch date for beta is May 12 on Facebook, we can't quite change it now. ;)

"This should be easily fixable by moving factoring stuff out into different methods."

Not sure what you mean here, as the callback is a Closure. I have tried to use curry to set the parameters to correct values, but I think the curried Closure that gets returned is now no longer the "same" handler and therefore still not unregistering it. Is there an unregisterAll(address)?? That would be perfect. ;)

Thanks again for your help and time.

Mark

[Tim Fox]

Can't you set the event bus handler as a member variable of the class, then it will be available in both the timer handler and the event bus handler itself? I think there are many ways this could be refactored...

[bytor99999]

If we put them all in a Map with a key. Because there is only one instance of SimpleGameTimerManagerImpl and many timers from many games setting through it.

Thanks

Mark

[bytor99999]

I think the issue is still that this handler/closure is only for a short period of time, as long as the timer. And that it is to cancel the timer, which cancelTimer require knowing the timerID. Which isn't created till after starting the timer, and is needed inside the handler/closure. But I can't create the Closure then somehow set a member variable inside a Closure, I can only curry or method scope it, and then I can't have a reference to the Closure to send to the unregisterHandler call. I am still stuck on that. However, in another area of code for something else, we have a Map of CLosures as in

final Map<String, Closure> handlers = new HashMap<>()

And look up the Closure by the address, in which we don't use the timerID in the address, instead we use a UUID as the address.

Mark

[Tim Fox]

On 02/05/15 22:02, bytor99999 wrote:

If we put them all in a Map with a key. Because there is only one instance of SimpleGameTimerManagerImpl

ouch.

In that case you could use AtomicReferences or create a holder class which you set various fields on and is accessed by the handlers. Or you could use an array, or maybe AtomicReferences. There are lots of ways this could be done.