koorchik / node-mole-rpc

Transport agnostic spec compliant JSON RPC client and server
MIT License
61 stars 13 forks source link

Cancel timer when server responds #13

Open yoursunny opened 4 years ago

yoursunny commented 4 years ago

The code has a bug: the program below does not terminate right away after all requests have been processed, but has to wait for the 300-second timeout. This patch fixes the bug by canceling the timer when server responds.

const MoleClient = require('../MoleClient');
const MoleServer = require('../MoleServer');

const TransportClient = require('./TransportClient');
const TransportServer = require('./TransportServer');

const EventEmitter = require('events');

(async () => {
    const emitter = new EventEmitter();
    const server = new MoleServer({
      transports: [new TransportServer({ emitter, inTopic: 'c', outTopic: 's' })],
    });
    server.expose({ f: () => 1 });
    await server.run();

    const client = new MoleClient({
      requestTimeout: 300000,
      transport: new TransportClient({ emitter, inTopic: 's', outTopic: 'c' }),
    });
    await client.callMethod('f', []);
    await client.runBatch([
      ['f', [1]],
    ]);
    console.log('program should exit now');
})();
koorchik commented 4 years ago

@yoursunny, thank you for the pull request! Will release the update during the week.

koorchik commented 4 years ago

I am not sure. That it should work as you have described. If we will stop the server when there is no active request then it should stop right after line await server.run(); which does not make a lot of sense. Moreover, if I run a server, I do not want it to stop if it is waiting for external calls. Now server behave the same with any "requestTimeout" values. It just waits for incoming calls.

yoursunny commented 4 years ago

The change is on the client. No change has been made to the server. When the client receives a response from the server, it should cancel the timer and allow itself to exit.

koorchik commented 4 years ago

Oh, clear. Then it should be separate processes for client and server. I will recheck. Thanks

yoursunny commented 4 years ago

For this test case, the server cannot prevent the process from exiting, because it uses an EventEmitter that does not have a socket.

Your socket based servers, such as the WebSocket transport, lack a close() method. That’s why you have to use process.exit() in test suites. But that’s a different topic.

koorchik commented 4 years ago

Yes, you are right. No IO in event emitter. Will try your example once more)

yoursunny commented 3 years ago

Hey @koorchik can you review and merge?

yoursunny commented 3 years ago

After a long time without action from the repository owner, I have decided to publish my patches in a fork: @yoursunny/mole-rpc. If anyone else is affected from this bug, you can try my fork.