senecajs / seneca-transport

Seneca micro-services message transport over TCP and HTTP.
MIT License
63 stars 45 forks source link

Possible reconnect Issues #91

Closed mcdonnelldean closed 8 years ago

mcdonnelldean commented 8 years ago

I have updated the tests with the reconnect test as written by @geek. This all seems to work with Master but I accept the fact that the test might be wrong. @StarpTech Could you run your eye over the client and server in /reconnect. It passes but it may not match with your reported test.

I want to keep this issue here so I can track it.

StarpTech commented 8 years ago

Hi @mcdonnelldean what is the status about this issue? Wil it be fixed in the next time? Unfortunately, I have run out of time now but I will try to do it asap. Thanks.

mcdonnelldean commented 8 years ago

Provisionally fixed but will be rechecked before 3.0 release in June as I think we may be missing an edge case.

Kindest Regards,

Dean

On 9 May 2016, at 08:56, Dustin notifications@github.com wrote:

Hi @mcdonnelldean what is the status about this issue? Wil it be fixed in the next time? Unfortunately, I have no time to investigate in it. Thanks.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub

StarpTech commented 8 years ago

Hi @mcdonnelldean is there any chance to fix this issue?

StarpTech commented 8 years ago

@mcdonnelldean I would appreciate it if you could tell me more details. The last decision is to change all services to HTTP but this is a bad step back in using seneca.

mcdonnelldean commented 8 years ago

@StarpTech From my point of view the Test added by @geek and confirmed by you has been added and is working. This leads me to believe that either the test isn't showing the issue or we missed something, hence my note above.

Is it still giving you issues? If so, could you update your little sample you wrote to latest and see if it still breaks, if so we know the test is not correctly covering the issue and I can add more.

As I said though, the original test added is going green so any more info from you would be very much appreciated.

mihaidma commented 8 years ago

The truth might be in the middle :) Tests pass on seneca 2.x but fail on seneca 1.x: https://travis-ci.org/senecajs/seneca-transport

I pushed a branch with a partial fix but I didn't finish it: https://github.com/senecajs/seneca-transport/tree/testfix

StarpTech commented 8 years ago

Issue is still open https://github.com/senecajs/seneca/issues/318 we test it against 2.1.0

mcdonnelldean commented 8 years ago

@mihaidma Glen's changes were to fix this. Can we get @thekemkid to look at this test? https://github.com/senecajs/seneca-transport/blob/master/test/tcp.test.js It is skipped for now because it was unreliable on travis.

We should have some tests to check if the change did indeed work. Once these are in publish a new version of Transport.

dgonzalez commented 8 years ago

@StarpTech hey, I am actually having a look to this. So far, It didn't happen to me using http instead of tcp as type in the options to create client and server:

'use strict';

var seneca = require('seneca');

var service = seneca();

service.client({
    type: 'http',
    port: 12345,
    host: '0.0.0.0',
    pin: 'role:foo'
});

service.act({
    role: 'foo',
    cmd: 'bar',
    name: 'tobi',
}, function(err, res) {
    console.log(res, Date.now());
});

And:


var seneca = require('seneca');

var service = seneca();

service.add({
    role: 'foo',
    cmd: 'bar'
}, function(args, cb) {
    console.log(args.name, Date.now(), args.meta$.id);
    cb(null, { test:123, name: args.name });
} )

service.listen({
    type: 'http',
    port: 12345,
    host: '0.0.0.0'
});

process.on('SIGINT', function() {
    process.exit();
});

process.on('SIGTERM', function() {
    process.exit();
});

process.on('exit', function() {
    console.log('exit');
    service.close();
})

Just works. However, I can see a difference between using TCP and HTTP: HTTP disconnects after sending the act whereas TCP keeps connected preventing the client from finishing. Looks to me like it should disconnect and terminate the client.

Does that makes sense?

mcdonnelldean commented 8 years ago

Merging with #114 Which has detailed steps generated from this discussion.