Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle empty message part better. #27

Merged
merged 1 commit into from
Aug 16, 2012
Merged

Handle empty message part better. #27

merged 1 commit into from
Aug 16, 2012

Conversation

gjohnson
Copy link
Collaborator

See inline comment(s).

@@ -43,7 +43,7 @@ RepSocket.prototype.onmessage = function(sock){
var envelopes = [];

for (var i = 0; i < msg.length; ++i) {
if (0x00 === msg[i][0]) {
if ('\u0000' === String(msg[i])) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If non-binary data is used the original implementation bombed out because that would be undefined (assuming strings, etc).

Copy link
Owner

Choose a reason for hiding this comment

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

hmm tests fail for me:

/Users/tj/projects/axon/node_modules/should/lib/should.js:125
      throw new AssertionError({
            ^
AssertionError: expected '\u0000' to have a property 'cmd'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmmmmm crazy, mine pass every time lol Did the test get merged and not the fix? Cause that sounds like the error that would happen in that scenario.

Copy link
Owner

Choose a reason for hiding this comment

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

looks like it got in fine haha, weird I'll check it out after work

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am still so confused on this... Do you get the error even when your seclude the test via TESTS=./test/test.whatever?

Copy link
Owner

Choose a reason for hiding this comment

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

yup if I run it directly I get that as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

crazy... just checked out directly from your master into tmp and all tests still pass over here... when you get a chance can you log out what the multipart buffer looks like:

RepSocket.prototype.onmessage = function(sock){
  var self = this;
  return function (msg, multipart){
    if (!multipart) return debug('rep expects multipart');
    var envelopes = [];

    console.log('raw msg:', msg);

    for (var i = 0; i < msg.length; ++i) {
      if ('\u0000' === String(msg[i])) {
        envelopes = msg.splice(0, ++i);
      }
    }

    console.log('app msg:', msg);

    self.emit.apply(self, ['message'].concat(msg, reply));

    function reply(){
      var args = [].slice.call(arguments);
      sock.write(self.pack(envelopes.concat(args)));
    }
  };
};

I correctly get:

raw msg: [ '\u0000', { cmd: 'hello' } ]
app msg: [ { cmd: 'hello' } ]

Copy link
Owner

Choose a reason for hiding this comment

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

wtf weird it works fine now, maybe it was on my retina, or maybe im just crazy hahaha

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

haha yay! I will get some other people to run it tomorrow, though I am using it constantly on a staging app and have had no probs yet... so maybe your just crazy... sorry! :-)

Copy link
Owner

Choose a reason for hiding this comment

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

must be! hahaha, I was probably running an older SHA or something, no clue

tj added a commit that referenced this pull request Aug 16, 2012
Handle empty message part better.
@tj tj merged commit a7dd443 into tj:master Aug 16, 2012
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