NodeFly / uhura

Server-to-server event emitter with session support
MIT License
19 stars 5 forks source link

Fixes for issue #11 #12

Open bnoordhuis opened 10 years ago

bnoordhuis commented 10 years ago

This is mostly complete and passes almost all uhura tests (see below) and all strong-agent tests.

I'm not entirely happy with the magic 'corked' property but I copied the concept from the 'ready' property. Both are setters that perform all kinds of side effects.

I'm still investigating the occasional failures of the 'should not lose messages while disconnected' test. It worked by accident before but 3475fff exposed a race condition.

There is at least one bug in the test itself - it's possible for the timer to expire between the last message being sent and received - but it sometimes misses an older message whereas a newer one has been received. Could indicate that there is more than one way for #11 to happen.

Suggested reviewers: @sam-github @themitchy

slnode commented 10 years ago

Refer to this link for build results: http://ci.strongloop.com/job/uhura/55/

bnoordhuis commented 10 years ago

it sometimes misses an older message whereas a newer one has been received

Update: the missing message is sent and received but the receiver never acks it for some reason. Needs further investigation.

Qard commented 10 years ago

update: pointed the links at your fork.

The "connect" event should not be emitted before the "start" event has been sent and processed by the server. See these two lines:

https://github.com/bnoordhuis/uhura/blob/989a3c0/lib/client.js#L167 https://github.com/bnoordhuis/uhura/blob/989a3c0/lib/client.js#L208

The server is supposed to send the connect event to the client so it isn't flagged as ready until the 'start' message sent at initialization has been received and processed. It's less a "connect" event and more a "no really, I'm actually connected now" event.

The settings to enable acks and object mode could be applied after the next message is received, if resuming a session through this branch here;

https://github.com/bnoordhuis/uhura/blob/989a3c0/lib/connection.js#L67

You should be able to just delete the line emitting the connect event from the client, actually.

slnode commented 10 years ago

Refer to this link for build results: http://ci.strongloop.com/job/uhura/56/

bnoordhuis commented 10 years ago

@Qard Thanks for the comments. I removed the client event. As to this comment:

The "connect" event should not be emitted before the "start" event has been sent and processed by the server.

Sorry, not sure I follow. Are you saying that's happening now? With this PR or in general?

sam-github commented 10 years ago

Looks OK to me.

Qard commented 10 years ago

@bnoordhuis The way it was set up originally was that the client socket would connect to the server socket and the server emitter would send a "connect" event back to the user after it received the start event and processed any setup stuff it needed, like loading sessions. I probably could have picked a different word than "connect", but it seemed more consistent from the user perspective.