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

Limit number of open sessions #165

Merged
merged 5 commits into from
Jan 19, 2024
Merged

Conversation

lthibault
Copy link
Contributor

📝 Summary

Use a semaphore channel to limit the number of open sessions. This is a hack because we're relying on session timeout behavior. Recommend ref-counting API that releases refs when block-height changes in near-term future.

Resolves #153

📚 References


  • I have seen and agree to CONTRIBUTING.md

This is a hack because we're relying on session timeout behavior.
Recommend ref-counting API that releases refs when block-height changes.
@lthibault lthibault added the enhancement New feature or request label Jan 18, 2024
@lthibault lthibault self-assigned this Jan 18, 2024
@lthibault lthibault mentioned this pull request Jan 18, 2024
1 task
@lthibault
Copy link
Contributor Author

Test passes locally ... re-running 🧐
I think this is ready for review in the meantime, though.

@lthibault lthibault marked this pull request as ready for review January 18, 2024 23:29
@lthibault
Copy link
Contributor Author

lthibault commented Jan 18, 2024

I'm noticing that failing test is potentially non-deterministic. Did I break something, or was this always flaky?

Test passes on my laptop, so I am unable to easily answer this with git bisect.

@metachris
Copy link
Contributor

tests passed now. sadly, geth tests are notoriously flaky.

Copy link
Collaborator

@ferranbt ferranbt left a comment

Choose a reason for hiding this comment

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

LGTM! Could you add a test?

@metachris
Copy link
Contributor

nice code and comments. is there a good way to write a test ensuring it works as expected?

@lthibault
Copy link
Contributor Author

Added unit tests. @metachris I think it's a good idea to hook in a envvar flag here, but I'd like to do it in a separate PR because it'll affect a different set of modules. Merging now and following up with that change.

@lthibault lthibault merged commit 669b321 into main Jan 19, 2024
3 checks passed
@lthibault lthibault deleted the feat/max-concurrent-build-sessions branch January 19, 2024 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Max Builder Sessions
3 participants