Removed detach #576
Removed detach #576
Conversation
MarcusLongmuir
force-pushed
the
remove-detach
branch
from
93c2d1a to
7c9ca21
Compare
|
@MarcusLongmuir what is the status of this PR? I recall you mentioning it may improve performance, is that still the case? |
MarcusLongmuir
force-pushed
the
remove-detach
branch
from
f6f9611 to
92f7873
Compare
|
@jonny-improbable: I've just managed to get this passing tests and it's now ready for review. |
| @@ -233,6 +246,7 @@ describe("FakeTransportBuilder", () => { | |||
| const onEndSpy = jest.fn(); | |||
|
|
|||
| const transport = new FakeTransportBuilder() | |||
| .withManualTrigger() | |||
| @@ -20,6 +21,7 @@ describe("FakeTransportBuilder", () => { | |||
| it("should allow the response trailers to be stubbed", done => { | |||
| const expectedTrailers = new grpc.Metadata({ foo: "bar" }); | |||
| const transport = new FakeTransportBuilder() | |||
| .withManualTrigger() | |||
There was a problem hiding this comment.
I see you've made it be a manual trigger, but I don't see you triggering it anywhere in the test? I don't follow, you please explain this change?
| @@ -258,8 +272,6 @@ describe("FakeTransportBuilder", () => { | |||
|
|
|||
| transport.sendHeaders(); | |||
|
|
|||
| // Note that we must defer the assertions as the grpc-web package detaches all callbacks, so they | |||
| // will not run immediately. | |||
| setTimeout(() => { | |||
There was a problem hiding this comment.
You've removed the comment, but not the timeout :)
|
Hi guys -- first, thanks for your work on this library, it's great! I was having an issue where I would occasionally get errors logged in the console associated with connections that were being closed and which were also receiving data from a (server) streaming grpc call at the same time. It doesn't happen all the time but does happen frequently when those conditions are met, maybe a race condition of some sort? Not sure. Here's an example of the error logged in the browser console:
To try to figure out what was going on, I cloned the repo. Using the code in the master branch, I can't reproduce the error, whereas I can reproduce it consistently with the 0.12.0 code. Looking at the commits in master since 0.12.0, this looks like the potentially relevant change. So, I wanted to let you know this latest code seems to be addressing my issue -- thanks! -- and also to ask whether you're considering releasing this code anytime soon? |
|
@galenwarren: We've just released |
|
Awesome -- thanks! |
This PR removes the
detachfunction and selectively replaces its usage.The
detachfunction was used to isolate user-provided callbacks from the rest of thegrpc-websuch that exceptions thrown by user-provided callbacks would not be caught bygrpc-web. It called the user-provided callbacks inside asetTimeoutcall to prevent the catching of exceptions from the callback function. In my own usage ofgrpc-webwith fake transports and alike this proved frustrating due to the disconnected stack trace caused by thesetTimeout.The function was also overused as a protective measure inside transports and that usage has been removed completely.
The usages that wrapped user-provided callbacks have been replaced by a
try+catchthat usessetTimeoutto re-throw the exception if the callback throws one, but otherwise will execute the callback immediately.This is a breaking change.
Usage that assumed the callbacks provided to
grpc-webwould be executed in a deferred manner could be broken by this change. This is unlikely to affect production code as the network request will always be asynchronous and would cause the callback to be invoked sometime in the future, but tests that assume other code will run between thegrpc-webinvocation and the callback being called could be affected.