added binary support #11

Merged
merged 1 commit into from Jul 7, 2012

Projects

None yet

2 participants

@kilianc

I completely rewrote the parser in order to be able to send Buffers as messages.
Now it is faster because we know each message how much is long and we don't have to try/catch every data event.

I hope the style is consistent with yours.

My only concern is about the node requirement that now is v0.7.x because of Buffer.concat.
If you want we can do it the old way.

Also even if I provided new test, would be cool to move from vows to mocha the whole suite, I can do it.

If you're going to merge this, I need to update doc's too.

Ex

socket.send('image::large', fs.readFileSync('/my/awesome/path'))

P.S. my finally thought about this, is that we should be able to pipe all the things, so a 'start::data' event with a stream would be super awesome.

// adding this event will disable internal buffering for binary messages.
socket.on('start::data', function (stream) {
  stream.pipe(fileStream)
})
@kilianc

Also there is a new dependency bufferjoiner a silly buffer utility that i wrote.
I don't know if you like it :)

@0x00A 0x00A merged commit 562f0e5 into foreverjs:master Jul 7, 2012
@0x00A

I will test in my dev env before i publish to npm.

@kilianc

Let me know how it goes.

@kilianc kilianc commented on the diff Jul 7, 2012
lib/nssocket.js
+NsSocket.prototype._fetchBody = function _fetchBody(chunk) {
+ var raw, event, data;
+ var chunkLength = chunk.length;
+ var bytesLeft = (this._eventLength + this._messageLength) - this._bufferJoiner.length;
+
+ if (chunkLength >= bytesLeft) {
+ raw = this._bufferJoiner.add(chunk.slice(0, bytesLeft)).join();
+ event = JSON.parse(raw.slice(0, this._eventLength));
+ data = this._messagetype ? raw.slice(this._eventLength) : JSON.parse(raw.slice(this._eventLength).toString());
+
+ this._eventLength = -1;
+ this._messageLength = -1;
+ this.emit(['data'].concat(event), data);
+
+ if (chunkLength - bytesLeft) {
+ process.nextTick(this._fetchHeader.bind(this, chunk.slice(bytesLeft)));
@kilianc
kilianc Jul 7, 2012

I think this is not working.

Could it happen that scheduling this on the next tick, a new data event fires before our _fetchHeader call?
This will break the header. ;/

@0x00A
0x00A Jul 7, 2012
@kilianc
kilianc Jul 7, 2012

it is 100% broken, If you write to the socket like 3 messages at the same time (same data event) it will take a wrong path. It needs to be sync.

@0x00A
0x00A Jul 7, 2012
@kilianc
kilianc Jul 7, 2012

It should be working replacing process.nextTick with the actual call. I'll PR again with more tests.

@0x00A
0x00A Jul 7, 2012
@kilianc

fixed, porting test to mocha and adding coverage. PR in ~3h

@kilianc

I fixed the bug, now is totally working.
TLS tests are not completely passing so I hope that you can pull from here and figure it out what is wrong.

@0x00A

ok, as soon as your tests are passing I can merge this pull request :)

@kilianc

Yes of course, this is why I didn't formally requested the pull, I was talking about help me understand why, even old test are not passing.

Also, can you explain motivations behind this? I started a new branch using directly tls.connect and it works like a charme. I didn't expose the function constructor, but just connect/createConnection and createServer ad it looks more consistent to me.

Thoughts? :D

@kilianc

coverage

branch https://github.com/kilianc/nssocket/tree/clearstream

I changed a lot of things:

  • removed commom/* hacks for tls
  • added ondata offdata oncedata methods
  • added createClient method
  • exposed write method
  • global refactoring

Also I stripped out semicolons, but I totally understand if you do not agree. I can revert this.

Can I open a PR?

@0x00A

Yea def! Also, as much as i personally dont care about semis our policy is to write "as common code as possible". Nice work on this, looking good so far.

@kilianc

Tell me if I am wrong, but this line should be removed.

// https://github.com/kilianc/nssocket/blob/clearstream/lib/nssocket.js#L203
this.retry.wait = this.retry.interval * Math.pow(10, this.retry.retries)

we are asking for retry.wait and we will overwrite every time

this.retry = options.reconnect ? {
  retries: 0,
  max: options.maxRetries || 10,
  interval: options.retryInterval || 5000,
  wait: options.retryInterval || 5000
} : false

what about

setTimeout(tryConnect, this.retry.wait * this.retry.retries)
@kilianc

Can you please explain what both interval and wait are supposed to be? :D

@0x00A

<3 blame. yes, we should set a simple timeout that is assigned to an instance variable so it can be cleared in case connect is called in succession.

@kilianc

here https://github.com/kilianc/nssocket/tree/clearstream!

I was updating docs too, but I think you guys have a standard/policy for that, if not I can do it, in this case it will need some engrish review :)

@0x00A

can you update this pull request including all your changes and submit? I'll review :)

@kilianc

@hij1nx I just pushed to the pull request branch (buffer) but nothing happened here, I think I have to open a new one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment