DVLP / signalr-no-jquery

120 stars 78 forks source link

Connecting Message Buffer Not Functioning Properly #69

Open alexhebert90 opened 6 months ago

alexhebert90 commented 6 months ago

Overview

From what I can tell, any messages that are received and stored in "ConnectingMessageBuffer" (messages received while signalR is in a "connecting" state) will never be received by the application.

Establishing Context

signalR.js

In the SignalR portion of the library, there is a concept of a ConnectingMessageBuffer that buffers messages that come in while signalR is in a "connecting" state. Later, after connection, those messages are flushed out so that the application still receives them.

In signalR.js, lines 350 - 359 are the relevant bits of code here, where $(this) gets called, that result being cached in the $connection variable, and that $connection variable being used at line 358 to trigger the onReceived event once the buffer has been flushed.

-- All of the above, by the way, is fine, but what's noteworthy is the detail I mentioned about $connection being captured and referenced in the code that will eventually distribute those flushed buffer messages to the application.

jQueryShim.js

Note that above, $connection was established by calling $(this) which maps to the jqueryFunction function in jQueryShim.js (line 13).

Notice the first line of jQueryFunction, line 14, where events is assigned to subject.events || {}. This, I believe is the source of the bug. From what I can tell, due to the order things happen, that ConnectingMessageBuffer callback gets set up before any events have actually been added to the connection. In theory, this should be fine, because the events get added to subject, which is a consistent reference to the connection in this case. However, if jQueryFunction gets called early, where events is still empty, and that result is "cached" (captured in this case), events will also be captured permanently as {} instead of being a reference to subject.events, which will update over time.

Proposed Solution

events at line 14 in jQueryShim.js needs to not be a variable that gets captured, but needs to always reference subject so that the events stay up to date. Note that I'm assuming this was just a mistake - a convenience to not have to repeat subject.events || {} over and over. Avoiding repetition is good, but in this case events is getting captured as a potentially stale value. Note also that I tested a very, very simple version of this fix and it appeared to fix the bug.

DVLP commented 6 months ago

Thanks for this detailed report. Feel free to raise a PR if you believe that this is a necessary fix