Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cancel timer when server responds #13

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

yoursunny
Copy link

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
Copy link
Owner

koorchik commented Jun 7, 2020

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

@koorchik
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

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

@yoursunny
Copy link
Author

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
Copy link
Owner

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

@yoursunny
Copy link
Author

Hey @koorchik can you review and merge?

@yoursunny
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants