search

metarhia / jstp

Fast RPC for browser and Node.js based on TCP, WebSocket, and MDSF
https://metarhia.github.io/jstp
Other
142 stars 10 forks source link

lib: add utility for call messages resending #320

Closed belochub closed 6 years ago

belochub commented 6 years ago

Callbacks passed to jstp method calls will now return an error when the connection is being closed. Since it is known, that callback messages are not being resent on connection drop, a special method on Connection was added that resends call message on connection to the server, and thus can be used for making a call to idempotent methods on the server.

I would like to request some help with tests from @nechaido. Also, there is a question about how we should deal with calls to remote methods done via RemoteProxy, I don't think that we should definitely resend all of the calls, but we should probably provide some possibility for the user to indicate that the call must be resent. Does anyone have any idea on how this should be done?

belochub commented 6 years ago

Ping @lundibundi.

lundibundi commented 6 years ago

@belochub @nechaido I do not see a reason behind all of the complexity of resendable-call-from-client.js test. As I understand it's supposed to test resending of calls that were sent at a point where client assumed that connection was still active but the client did not get the reply for one reason or another. So why not just make this test the same as resendable-call-from-closed-client.js but instead of calling connection.close() close the server-side connection there via server.getClientsArray()[0].close();, this way at a point of sending a call the client still has an active connection (from his point of view) and the server will not receive this call?

lundibundi commented 6 years ago

Also, I've been thinking about resending calls that have not been received by the other party, won't we make the same call twice this way? So the example is as follows if we make a callMethodWithResend and after it, the connection breaks so that the other party didn't receive the call but it was buffered to be sent again on this side. As this side's connection was closed _dismantleCallbacks was called and another call was buffered. Hence upon reconnecting the session will resend both the original call (because the other party notified that it didn't receive it by sending appropriate receivedMessagesCount) and the one after it but as the original's call callback was removed only one callback will be called, but we still have 2 calls. I have slightly modified resendable-call-from-closed-client.js test by replacing connection.close() with server-side socket destroy and was able to witness the problem I've described above.

nechaido commented 6 years ago

@belochub @nechaido I do not see a reason behind all of the complexity of resendable-call-from-client.js test. As I understand it's supposed to test resending of calls that were sent at a point where client assumed that connection was still active but the client did not get the reply for one reason or another. So why not just make this test the same as resendable-call-from-closed-client.js but instead of calling connection.close() close the server-side connection there via server.getClientsArray()[0].close();, this way at a point of sending a call the client still has an active connection (from his point of view) and the server will not receive this call?

Thank you, I have added the corresponding test.

Also, I've been thinking about resending calls that have not been received by the other party, won't we make the same call twice this way? So the example is as follows if we make a callMethodWithResend and after it, the connection breaks so that the other party didn't receive the call but it was buffered to be sent again on this side. As this side's connection was closed _dismantleCallbacks was called and another call was buffered. Hence upon reconnecting the session will resend both the original call (because the other party notified that it didn't receive it by sending appropriate receivedMessagesCount) and the one after it but as the original's call callback was removed only one callback will be called, but we still have 2 calls. I have slightly modified resendable-call-from-closed-client.js test by replacing connection.close() with server-side socket destroy and was able to witness the problem I've described above.

Fixed. It was the problem with the test itself.

lundibundi commented 6 years ago

@belochub basically the issue is with _dismantleCallbacks() and callMethodWithResend. As _dismantleCallbacks is currently called upon connection close, but at that time you don't yet know whether the user will not get a callback or he will (because the message might not have been send at all hence it will be resent as usual, see comment) hence it is incorrect to schedule resend of messages upon connection close. You will only know whether user will or will not get a callback when he/server sends back a number of messages it received. That's why in java implementation the error is actually reported upon connection restore.

nechaido commented 6 years ago

@lundibundi ping.

belochub commented 6 years ago

Landed in https://github.com/metarhia/jstp/commit/19402edc4863b7fddde5f538eda56a4ad0f59c21.