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

Load async casues deadlocks/thread starvation #1678

Closed
wants to merge 2 commits into from
Closed

Load async casues deadlocks/thread starvation #1678

wants to merge 2 commits into from

Conversation

joro550
Copy link

@joro550 joro550 commented Aug 14, 2020

Description

I was seeing an issue this week with dynamodb and the load async method after quite a lot of investigation (thanks to some tools) I found that the library is doing some sync operations over async, which causes som thread starvation issues and eventually deadlocks, there is a good issue that has been raised here: #1476

I thought I'd start adding async methods to some of the base libraries, I have no resolved the issue as of yet, but thought the sooner the better with this change

Motivation and Context

#1476

Fixing scaling issues with the dynamodb sdk

Testing

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have read the README document
  • I have added tests to cover my changes
  • All new and existing tests passed

License

  • I confirm that this pull request can be released under the Apache 2 license

@joro550 joro550 changed the title WIP: Load async casues deadlocks/thread starvation Load async casues deadlocks/thread starvation Oct 14, 2020
@github-actions
Copy link

github-actions bot commented Feb 2, 2022

We have noticed this issue has not received attention in 1 year. We will close this issue for now. If you think this is in error, please feel free to comment and reopen the issue.

@acraven
Copy link
Contributor

acraven commented Jul 30, 2022

@joro550 We've been seeing some performance problems that can be attributed to the synchronous DescribeTable call. While this PR may not cover the part of the API we're using having this PR approved and merged would allow us to progress.

@joro550
Copy link
Author

joro550 commented Aug 2, 2022

Hey @acraven sadly I don't have permission to actually merge this PR, I am not a member of the AWS team - I am but an employee at a company who uses AWS in a high load environment and trust me we too were having pretty big performance issues because of this particular piece of functionality and it seems that there are a few other issues been raised in the repo for this that are getting absolutely 0 traction with the client team.

I have completely given up with them (especially now that they had a bot close my PR) - what we learned INSTEAD is that the describe table will not run if it all ready has the information cached, so when you first boot your application if you run:

IDynamoDBContext context; // just here to show you which interface this belongs to
context.GetTargetTable<TableName>();

it should load the table into memory and not do the extra describe call under high load, looking at the code it seems to just be a dictionary that is assigned against the DynamoDBContext so I think if I were you I would make that class a singleton (so there is only one per application) and test that out to see if that helps the performance.

Just to give you a bit of an update - it did seem to improve the performance (or stability) for us but now we have issues with Assuming roles where it seems to do it under 5ms for 100+ calls but then shoots up to 200ms for 2 calls then back down 🤷‍♀️ I'll never fully understand why

@joro550 joro550 closed this Aug 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants