fil512
September 2, 2010, 1:37pm
5
swampwallaby:
Hi Ken,
Had a look at your fix. Unfortunately, it’s not really a bug fix, you’re just masking the reporting of a real bug, which will lead to unpredictable and probably unexpected behavior. In general, if SequenceEncoder.Decoder.nextXXXXX() throws an exception, it’s a bug in something and we want to report it, not hide it.
I guess I misunderstood you when you wrote “The best you can do in Decorator.mergeState() is to detect the problem and fail more gracefully.” By “fail more gracefully” you meant that we should continue to throw a stack trace, but with more information?
swampwallaby:
To answer your question from a previous email
What do you mean by “someone else’s client”?
This bug appears to occur when Player A Connects to the server and starts up a game. Player B connects to the server and presumably synchronizes with Player A so that now player B client has a copy of all the pieces that player A has with the same type and state. [As an aside, this is why the ‘type’ information is saved in the save game as well as the ‘state’ information - It is the only way to ensure that both clients are seeing the same pieces].
Now Player A issues a command to one of his pieces which causes it to change state and a ChangePiece command is create by the A client and sent via the server to the B client to force matching piece in the B client into the same state. When the B client goes to execute the ChangePiece command on the matching piece, the number of ‘states’ (i.e. the number of traits) in the piece in the B client appears to be different to the number of traits that where in the A piece, and a NoMoreElements exception is generated and we know that something bad has happened. [Note, this is a simplistic explanation, the situation that leads up to the bug may be infinitely more complex]
Currently in 3.1, the NoMoreElements exception causes a stack trace, a bug report and a nasty message to the players so that they know something has gone wrong. Hiding this fact is not a good idea. If the 2 clients have become de-synced, anything might happen. If this happens, the playrs need to know that the integrity of their game has been compromised - and hopefully they can reproduce the problem for us
Instead of hiding the message, we actually need to keep generating the bug report, but pump a lot more information into the report so that we can try and work out why the 2 clients got out of sync in the first place. Or they might not be out of sync, but the ChangePiece Command is being generated incorrectly. It may also be a bug in the trait code somewhere. At this point we do not know. What would be nice would be to see a ‘stacktrace’ of the current counter - Basically what you did in those JUnit tests you wrote. I’m thinking each trait should have a toString() method that returns your English expansion of the trait, and use this to generate a PieceTrace dump. Bug Reports generated by traits could then include the PieceTrace.
Some sort of direct dump of the State and Type would be useful as well.
There are plenty of other places where this information would be helpful.
I’m not sure how much of this we would want to put into 3.1, perhaps just some additional state/type info for this particular bug.
Brent,
Thanks for taking the time to explain this to me. I now have a much clearer picture of the context of this bug. I agree with you that we should continue to throw an exception, but include more information in the stack trace. I will back out my change and start over, taking the approach you suggest above.
Joel–I need to change the bug status to REOPENED. But I don’t see REOPENED in the list of choices…? I think we need a REOPENED status for situations like this–when a rookie coder thinks they fix a bug, but someone more experienced reviews the fix and sees an issue with it.
-K