Skip to content

Feature sync node graph fix #585

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.

Sign up for GitHub

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

Jump to bottom
Merged
tegefaulkes merged 5 commits into from
Oct 16, 2023
Merged

Feature sync node graph fix #585

tegefaulkes merged 5 commits into from
Oct 16, 2023

Conversation

@tegefaulkes
Copy link
Contributor

@tegefaulkes tegefaulkes commented Oct 16, 2023 •
edited
Loading

Description

This PR addresses some minor issues with syncNodeGraph and the multi connection logic.

Issues Fixed

  • No direct issues fixed, just a collection of small incidental fixes.

Tasks

  • 1. Fix issue with NodeManager.syncNodeGraph throwing when a connection fails.
  • 2. NCM establishMultiConnection should only throw if no connections were established.
  • 3. Expand tests relating to multi connection logic.
  • 4. NodeConnection throws a nodes domain timeout error on connection timeout.
  • 5. Something is holding the process open, related to the keepAliveIntervalTime. Investigate. Timer not cleaned up when cancelled. js-timer#15

Final checklist

  • Domain specific tests
  • Full tests
  • Updated inline-comment documentation
  • Lint fixed
  • Squash and rebased
  • Sanity check the final build

@tegefaulkes tegefaulkes self-assigned this Oct 16, 2023
@ghost
Copy link

ghost commented Oct 16, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@tegefaulkes
Copy link
Contributor Author

tegefaulkes commented Oct 16, 2023

Almost done with this, Just investigating task 5. So far as I can tell, the timer relating to the KeepAliveIntervalTimer is not being cleaned up when a connection ends and is left to time out. May need to be fixed is js-quic.

@tegefaulkes
Copy link
Contributor Author

tegefaulkes commented Oct 16, 2023

There's a weird bug. The process is being held open by a timer reference. The specific timer is the keepAliveIntervalTimer in QUICConnection. I've run some tests while monkey patching in some logging to the local dependencies and found that... while yes, the Timer IS being cancelled. the underlying timer is not being cleared. In fact, the Timer is not even rejecting after being cancelled.

This seems to be a problem @matrixai/timer. I'll need to investigate it more but I'm confident the problem is out of scope for this PR.

@CMCDragonkai
Copy link
Member

CMCDragonkai commented Oct 16, 2023

There's a weird bug. The process is being held open by a timer reference. The specific timer is the keepAliveIntervalTimer in QUICConnection. I've run some tests while monkey patching in some logging to the local dependencies and found that... while yes, the Timer IS being cancelled. the underlying timer is not being cleared. In fact, the Timer is not even rejecting after being cancelled.

This seems to be a problem @matrixai/timer. I'll need to investigate it more but I'm confident the problem is out of scope for this PR.

Can you make an issue, and how to reproduce this in js-timer.

@CMCDragonkai
Copy link
Member

CMCDragonkai commented Oct 16, 2023

How do you know the underlying timer is not being cleared. Did you check by logging out the underlying timer object that is inside nodejs?

@CMCDragonkai CMCDragonkai mentioned this pull request Oct 16, 2023
Merged
14 tasks
@tegefaulkes
Copy link
Contributor Author

tegefaulkes commented Oct 16, 2023

I added a console.log just before each clearTimeout() and never saw the output when testing. When I change the timeout delay I see that reflected in the delay for the process closing. I added a cheeky clearTimeout to the Timer.cancel() method and the process started ending immediately after the test ended. When I added a .finally() to the Timer` it never got called after the timer was cancelled.

It's pretty clear that the underlying node timer in Timer is getting leaked here.

@CMCDragonkai
Copy link
Member

CMCDragonkai commented Oct 16, 2023

Ok please create a new issue js-timer, and task 5, shouldn't just be ticked if it wasn't resolved, you can however link to the new issue.

@tegefaulkes tegefaulkes mentioned this pull request Oct 16, 2023
Closed
@tegefaulkes
Copy link
Contributor Author

tegefaulkes commented Oct 16, 2023

New issue created at MatrixAI/js-timer#15

@CMCDragonkai CMCDragonkai mentioned this pull request Oct 16, 2023
Closed
@tegefaulkes tegefaulkes mentioned this pull request Oct 16, 2023
Closed
@tegefaulkes tegefaulkes merged commit 2066515 into staging Oct 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@tegefaulkes tegefaulkes

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tegefaulkes @CMCDragonkai