Skip to content
This repository has been archived by the owner on Apr 24, 2020. It is now read-only.

revert commit #ca6d867 (from 2.8 tag) — req reconnection fail on router restart #396

Closed
wants to merge 1 commit into from

Conversation

yamsellem
Copy link

This is a revert of commit #ca6d867 from @utvara remove receive message delayed handling which introduces an issue (affecting every version since 2.8.0).

A test is provided to reproduced the problem we have encountered in production: a req socket is connected to a router. When this routerrestarts (in our case, mainly on version bumping), the req is not able to reconnect. What we presume, is that one request can be lost — the req socket is awaiting for a response, but, because the router was restarted in the meantime, this precise request will never get an acknowledgement. Thus blocking the req socket which needs an ack to this request to send next ones.

@utvara, can you please explain why you have change this code on 19 Aug 2014?
Is anyone else experiencing the same issue?

@yamsellem
Copy link
Author

I don't get why Travis' build is failing, every test run smoothly locally.

@yamsellem
Copy link
Author

It's been 14 days since this issue/pull request has been suggested.
Is it possible to have a status on this, please?

@ronkorving
Copy link
Collaborator

I don't get why Travis' build is failing, every test run smoothly locally.

Status is, this PR (whether an improvement in one way or another) is not ready to be merged.

Looking at the PR and the issue you describe, I fail to see how reverting the nextTick should help? Perhaps in an in-process test scenario it would. But does it also solve it between processes? I don't quite understand the solution here. I do understand the problem you describe btw.

@yamsellem
Copy link
Author

May I ask why nextTick was there in the first place?
Do you imagine another way to deal with this reconnection failure?

@ronkorving
Copy link
Collaborator

iirc, it was there because it "magically" solved some issues a long, long time ago. After several refactoring phases, it was deemed no longer necessary and was removed. No regressions have been discovered so far, so it was kept this way. Well, no regressions... until this PR?

It will require some digging in past year's commits to find out who did what, to be honest (I was just here to merge, so don't quite remember who did what, sorry).

@ronkorving
Copy link
Collaborator

Closing this, as it should not have been an issue for some time. If there are any issues remaining, please open a new issue or PR.

@ronkorving ronkorving closed this Jun 13, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants