search

deepstreamIO / deepstream.io-client-js

The Browser / Node.js Client for deepstream.io
Other
294 stars 109 forks source link

fix: add isComplete flag to prevent callback being called more than once #553

Closed jaime-ez closed 3 years ago

jaime-ez commented 3 years ago

Possible fix for #551

yasserf commented 3 years ago

If the server is sending a response timeout that shouldn’t affect the client sending a response afterwards. Seems more like the server should respond with an unknown rpc correlation id

jaime-ez commented 3 years ago

The server is not sending a response timeout, if the client has a timeout less than the server one, the client locally calls the error here without sending a message to the server, which I think is fine becasue it is a local decision to define that the timeout has been exceded, but it wasn't declaring locally that the rpc has been finished, thus the "double callback" with the incoming server message. I propose that adding this flag will cause that the incoming server message, if sent, will be ignored.

yasserf commented 3 years ago

Gotcha.

I would think it makes more sense for the client not to do the response_timeout, and just let the server handle it. The issue is primarily around users configurations being invalid.

Silencing errors using guards is a tricky decision, because on one side we are making it work 'as expected', but on the other side we still have this RPC response being sent up all the way to the client which is invalid.

I would say the two things we can do is: 1) Send the required server config to the client (like heartbeat/timeouts). The client can then validate it. 2) Just give better defaults to the client, where the response time is much higher than the server.

The RPCs should all be marked invalid if the connection to the server is lost, so there shouldn't be any reason to have timeouts client side that change state, we usually only use them to print warnings.

jaime-ez commented 3 years ago

So basically the client should'nt have any rpc timeout options (neither accept nor response) and just let the server handle it? I don't see the point on validating configs between server and client if we can not override the server values client-side.

yasserf commented 3 years ago

It isn't to validate config client side, its to set the config entirely for certain things on the client.

For example people are running into issues with the heartbeat on the server and client not matching.

In terms of the RPC timeout, it really doesn't matter much, the whole concepts of timeouts that are baked into deepstream since v0.0 don't do much other than log warnings. As long as the connection state is properly maintained (by using correct heartbeat logic) then the user will either get the timeout from the server or the connection will be considered lost and will trigger error callbacks.

I would say we just delete timeout issues on the client as long as the connection works.

jaime-ez commented 3 years ago

For example people are running into issues with the heartbeat on the server and client not matching.

Have those issues been solved?

I would say we just delete timeout issues on the client as long as the connection works.

Yes I agree, less config is better. I'll update with changes reflecting this.

however I'm not sure on the acceptTimeout...could it be that in some case the rpc is never accepted? By removing that timeout on the client we are making the whole RPC_ACTION.ACCEPT message irrelevant or I am missing something?

yasserf commented 3 years ago

The accept timeout is used by the server to indicate a backend rpc provider hasn't confirmed it is willing to satisfy the request, which the server takes as a sign to try another one.

Generally I would argue that sort of logic is a bit extreme, if you make a request and it fails then the server should just pass on that it failed to respond, to avoid having the same thing processed twice. We don't tell the clients to abort either so the response goes into the void.

Like I said, timeouts are pretty excessively implemented, should have dropped more of them when doing the v4 refactor. We can use timeouts to warn that the server is being slow but it should just fail hard.

Have those issues been solved?

I believe they have gotten better, but there was a PR open somewhere than still needs to be merged.

jaime-ez commented 3 years ago

Pull updated:

Comments?

yasserf commented 3 years ago

This looks good to me. Do we have any tests to make sure that when connection drops the RPCs fails?

jaime-ez commented 3 years ago

This looks good to me. Do we have any tests to make sure that when connection drops the RPCs fails?

apparently yes: https://github.com/deepstreamIO/deepstream.io-client-js/blob/master/src/rpc/rpc-handler.spec.ts#L553

yasserf commented 3 years ago

Apologies for being nitpicky in advance 🙈

The one you linked to tests what happens when the response is an error (so it goes to the provider, errors because of the provider, goes to deepstream and then back to the requestor).

The test that covers a network loss is here:

https://github.com/deepstreamIO/deepstream.io-client-js/blob/master/src/rpc/rpc-handler.spec.ts#L579

jaime-ez commented 3 years ago

yes I meant to reference that line...wrong copy/paste :+1: