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

Implements pending future tracking in async. #11181

Merged
merged 1 commit into from
May 8, 2019
Merged

Conversation

dom96
Copy link
Contributor

@dom96 dom96 commented May 5, 2019

This is incredibly useful for figuring out possible leaks caused by stuck async procs/futures.

Here is an example from my server during a leak:
Screen Shot 2019-05-05 at 21 35 25

You can see some counts at 6k+. Normally they are all 0 or 1. I will also be adding support for this in my prometheus package which will make analysing this an absolute dream


var futuresInProgress {.threadvar.}: Table[FutureInfo, int]

proc getFuturesInProgress*(): var Table[FutureInfo, int] =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole proc is not necessary with the new table implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest it's probably better to have this proc anyway. Accessing a global var doesn't seem like great API design.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What? The proc is exported

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use a TableRef then with this logic, this var Table here is fragile :P

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, can you explain why it is fragile?

lib/pure/asyncdispatch.nim Outdated Show resolved Hide resolved
@dom96 dom96 force-pushed the pending-future-tracking branch from baa155d to 1805947 Compare May 6, 2019 11:37
@dom96
Copy link
Contributor Author

dom96 commented May 6, 2019

Windows test failure is unrelated to PR. https://ci.appveyor.com/project/dom96/nim/builds/24332702/job/dtt16wof6xkd7qjx#L1407

@dom96 dom96 force-pushed the pending-future-tracking branch from 1805947 to abe634a Compare May 6, 2019 16:18
@dom96
Copy link
Contributor Author

dom96 commented May 6, 2019

And here is what this looks like in Prometheus:

Screen Shot 2019-05-06 at 17 37 50

@dom96
Copy link
Contributor Author

dom96 commented May 8, 2019

Is it really so risky to merge this that you need to keep retrying the CI until it passes?

@Araq Araq merged commit aa76857 into devel May 8, 2019
@Araq Araq deleted the pending-future-tracking branch May 8, 2019 18:37
@dom96
Copy link
Contributor Author

dom96 commented May 8, 2019

Thanks @Araq <3

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.

3 participants