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

Client always times out #169

Closed
alwhiting opened this issue Oct 19, 2024 · 8 comments
Closed

Client always times out #169

alwhiting opened this issue Oct 19, 2024 · 8 comments
Assignees
Labels
bug Something isn't working released on @develop

Comments

@alwhiting
Copy link

alwhiting commented Oct 19, 2024

Perhaps I'm missing something here; but the block starting at this line seems to be causing the client to constantly be reconnecting:

if (this._main._opt.connectionTimeout > 0 && (this._readyState === ReadyState.CONNECTED || this._readyState === ReadyState.CONNECTING)) {

Is there something that should be done to prevent this behavior? It's not possible to set the connectionTimeout to 0 so this is basically constantly triggering a socket destruction and then the retry logic kicks in. Also the error comes up as "server disconnected" but in reality it is us, the client, who disconnected, which makes me think maybe this is unintentional.

@Bugs5382
Copy link
Owner

@alwhiting Just so you know I been expecting this myself recently. You might have nailed it. Let me work on it this weekend.

@Bugs5382 Bugs5382 self-assigned this Oct 24, 2024
@Bugs5382 Bugs5382 added the bug Something isn't working label Oct 24, 2024
@Bugs5382 Bugs5382 mentioned this issue Oct 31, 2024
10 tasks
Bugs5382 pushed a commit that referenced this issue Oct 31, 2024
# [3.0.0-beta.1](v2.3.1...v3.0.0-beta.1) (2024-10-31)

### Features

* client connection ([e6ee6a3](e6ee6a3)), closes [#169](#169)
* client connection ([#175](#175)) ([aeb4108](aeb4108))

### BREAKING CHANGES

* Default now set to be always connected. Must set connectionTimeout, if you want your client to disconnect after a certain time. The max is still 60 seconds.
@Bugs5382
Copy link
Owner

🎉 This issue has been resolved in version 3.0.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Bugs5382
Copy link
Owner

@alwhiting I just pushed the changed into the develop branch. There should be a NPM that you should test out prior to me releasing it. I have to do some other internal testing, a lot of other series of tests that I run through not on any CI since this deals with medical applications. So let me know how your testing. By Monday, I will release on main.

@alwhiting
Copy link
Author

alwhiting commented Nov 1, 2024

I have been working around it with this hack for now:

class ClientBugWorkaround extends Client {
    constructor(opts: ClientOptions) {
        const copy = JSON.parse(JSON.stringify(opts));
        copy.connectionTimeout = 1000;
        super(copy);
        this._opt.connectionTimeout = opts.connectionTimeout || 0;
    }
}

That got around the normalize function enforcing a timeout; when it's set to 0 the timeout that was killing the socket was not run.

@Bugs5382
Copy link
Owner

Bugs5382 commented Nov 2, 2024

What’s the PR for this intergrated into the code?

@Bugs5382 Bugs5382 mentioned this issue Nov 2, 2024
10 tasks
@Bugs5382
Copy link
Owner

Bugs5382 commented Nov 2, 2024

@alwhiting Add it to PR #176 please :)

@alwhiting
Copy link
Author

@alwhiting Add it to PR #176 please :)

I didn't actually fork this one just did the hack I pasted above in my code so I could bypass the minimum value for the connectionTimeout option. In the short term I think you could allow that to be set to 0 instead of a minimum of 1000ms. In the longer term I think investigating why that interval exists that calls socket.destroy() is even there... maybe there is a reason for it but I'm not sure what.

@Bugs5382
Copy link
Owner

Bugs5382 commented Nov 2, 2024

sweet. I have already done that in develop:

connectionTimeout: 0,

assertNumber(props, 'connectionTimeout', 0, 60000)

and now its working like designed. codec is fixed in the node-hl7-package. merging in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working released on @develop
Projects
None yet
Development

No branches or pull requests

2 participants