Logic Flaw

[artdude543]

So according to Carina in the Promise.races we are expecting the reply packet to be given as well as a close but we won't get close all the time. And with the logic in the resolveOn method it will reject if the event is never fired from the socket. Thus leading to an error which is not actually valid, could also cause logic issues with applications extending this and not actually expecting the error when the connection is still open.

Also with some of the races the .catch is never created nor mitigated where the application using Carina can get it; leading to unhandled exceptions this ties into #16 somewhat. But after I noticed @GhostfromTexas with the timeout bug I dug into the code some more to find this new issue.

Examples of the close event being created:

• https://github.com/WatchBeam/carina/blob/master/src/socket.ts#L271
• https://github.com/WatchBeam/carina/blob/master/src/socket.ts#L355

I know the second one mitigates the event to a warning event, but do we really need this close check on each event sending? Or could it be re-done to not actually throw a reject each time for packets which do actually get a reply.

[jamesbirtles]

Promise.race will return the first promise that resolves (or rejects), throwing away the other promise. I'm not sure I fully understand the issue you are describing. Are you saying you're somehow getting unhandled rejections on socket close for packets that resolved correctly already?

[artdude543]

The issue is easily reproducible when you have a debugger attached to the process or log the rejection call. We all know how the race works but something is going on to also trigger it and then throw a rejection error.

For example I sub to an event Carina does it's thing gets a reply it's expecting that resolves but the close packet which is in the race will reject even if is the first call resolves fine. In normal runtime you won't see the error being thrown due to the location. But when you run an node application via an IDE which is injecting the debugged it will throw and builds fail.

I'm not sure how else I can explain this. @GhostfromTexas can chime in on this due to us debugging the issue on his last stream; because Visual Code was getting the rejecting and throwing out.

[jamesbirtles]

Does the close resolver throw only when the socket closes (even if it was well after the reply to the packet was received), or is it throwing when the socket appears to be fine?

What version of Node are you using, and are you using any third party promise library, e.g. Bluebird?

[GhostfromTexas]

I was able to reproduce this in a very simple test-case using both node 6.9.4 and 6.10.0 without any additional packages like bluebird

The exception is being thrown in Utils.resolveOn

The Promise Race call in socket.ts launches three Utils.resolveOncalls for events reply, cancel and close, each one of those starting a timer with "setTimeout" - When carina works properly, the "reply" event is handled and resolves the race condition, so (with the recent PR i made) the timers is halted for the event that takes place, but the other 2 timers started by the promise race call to Utils.resolveOn are still active, and eventually trigger causing "reject" to be thrown.

When working with the debugger attached in VSCode, you get this exception all the time.

[connor4312]

So according to Carina in the Promise.races we are expecting the reply packet to be given as well as a close but we won't get close all the time.

close is not expected, but it is a possibility.

And with the logic in the resolveOn method it will reject if the event is never fired from the socket. Thus leading to an error which is not actually valid... do we really need this close check on each event sending?

This is intended. If some error happens, on the network or server, we don't want to be leaking memory with a bunch of unresolved, un-GC-able promises waiting to be fulfilled. The close check is preferred since we want to explicitly cancel ongoing calls when the socket closes, rather than saying "oops, they timed out, we don't know what happened."

Also with some of the races the .catch is never created nor mitigated where the application using Carina can get it

That's a bug! 🐞

For example I sub to an event Carina does it's thing gets a reply it's expecting that resolves but the close packet which is in the race will reject even if is the first call resolves fine.

What I suspect is happening is, in order:

• You have a debugger attached. The timeout has elapsed and a reply has been received on the tcp listener while execution is paused, and then you play again.
• The ws module begins reading the message via on('data') from the socket.
• Node sees that the timeout has elapsed, the timeout function fires on the next tick and the promise gets rejected,
• The ws module completes reading the message and fires the reply event.
• The reply is effectively discarded since the promise already rejected.

While this does lead to some undesirable behavior while debugging, the overall behavior is intended. If the full message has not been read by the provided timeout, then an error is raised. There's no way for Carina to know in advance that a message is about to be read from the socket.

I suggest that, during debugging, you set the timeout to a much higher value (or Infinity) to avoid these errors.