Set transport handlers earlier in Typescript client #12524
Conversation
BrennanConroy
added
area-signalr
BrennanConroy
requested review from
analogrelay,
halter73 and
mikaelm12
as code owners
| .on("GET", () => { | ||
| httpClientGetCount++; | ||
| if (httpClientGetCount === 1) { | ||
| if ((connection as any).transport.onreceive && (connection as any).transport.onclose) { |
There was a problem hiding this comment.
This seems like a fragile way to test if the handlers are set considering it would fail if the private "transport" property gets renamed. Even the test name of "transport handlers set before start" seems overly specific to the implementation.
Can't we verify this in some more robust way? Like by sending a message as part of the first get response and ensuring the correct .on handler gets called?
There was a problem hiding this comment.
This seems like a fragile way to test if the handlers are set considering it would fail if the private "transport" property gets renamed.
Says the guy that did (hubConnection as any).serverTimeout(); in tests!
Even the test name of "transport handlers set before start" seems overly specific to the implementation.
Not sure what you mean here, we're literally testing the specific implementation so...
Can't we verify this in some more robust way?
I couldn't find an easy way (read: any way) to test this without grabbing internal fields. In general we don't test race conditions unless there is a way to force the race to happen. In this case I found a way to force it but have to grab the internal fields to verify they were set. If it makes you happy I can just remove the test.
Like by sending a message as part of the first get response and ensuring the correct .on handler gets called?
That doesn't work because the real first response is after connect returns to HttpConnection and by then it's too late as we'll be past the race.
There was a problem hiding this comment.
Says the guy that did (hubConnection as any).serverTimeout(); in tests!
I was actually wondering if you would bring that up when I wrote this. I'm impressed you remembered 😄
Sometimes pragmatism beats purity. I didn't want to force a client test to wait for a server timeout to be exceeded just to pass. I'm a stickler for making tests run quickly, so I thought it was a good tradeoff.
If this really is too difficult to test any other way, I'm fine with it. I'm not entirely convinced that this is a hard to force race condition. You just need a transport that calls one of the callbacks prior to the start promise completion. That would probably be easiest with a mock transport inside a unit test.
There was a problem hiding this comment.
You just need a transport that calls one of the callbacks prior to the start promise completion. That would probably be easiest with a mock transport inside a unit test.
So, we actually might want to have another test that does this, unfortunately it will hit a different code path than the main one as we check if someone passes in a custom ITransport and go down a different path for that case. But that doesn't help make this test nicer 😢
There was a problem hiding this comment.
I'm fine if we just test the custom ITransport in the unit test and leave this test as is.
BrennanConroy
force-pushed
the
brecon/tsRace
branch
from
41d4d5d to
6fc0803
Compare
|
🆙 📅 |
|
This comment was made automatically. If there is a problem contact [email protected]. I've triaged the above build. I've created/commented on the following issue(s) |
|
This comment was made automatically. If there is a problem contact [email protected]. I've triaged the above build. I've created/commented on the following issue(s) |
Fixes https://github.com/aspnet/AspNetCore-Internal/issues/2817
We were setting the handlers after start so there were a couple races.
One where data could be received from the server before the handlers were set and so they would be missed.
And another where stop could be called and cause a null ref on
this.transport!.onreceive = this.onreceive;if it happened at the "right" time.