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

Fix race issue #23

Closed
wants to merge 3 commits into from
Closed

Fix race issue #23

wants to merge 3 commits into from

Conversation

daveads
Copy link
Contributor

@daveads daveads commented Oct 3, 2024

fix: Resolve race condition in HTTP server handling

  • Implement thread-safe safeServer to wrap http.Server
  • Modify Serve function to use safeServer for improved concurrency
  • Ensure atomic operations for server start and shutdown

This change addresses a race condition in the HTTP server handling,particularly affecting CLI tests. By introducing a safeServer struct with atomic operations and proper synchronization, and prevent potential data races during server startup and shutdown.

Fix for terrastruct/d2#2087

@daveads
Copy link
Contributor Author

daveads commented Oct 3, 2024

@alixander reverted....

is it okay now ?

Copy link
Contributor

@alixander alixander left a comment

Choose a reason for hiding this comment

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

So what I was asking originally is whether this PR can be reverted: #19

If so, I'd like to see the revert in this PR.

@alixander
Copy link
Contributor

you seem to be reverting it in one commit, and then your next commit just undoes it. I'm confused what you're doing. Please stop closing and opening new PRs too

@daveads
Copy link
Contributor Author

daveads commented Oct 3, 2024

So what I was asking originally is whether this PR can be reverted: #19

If so, I'd like to see the revert in this PR.

oh... i tried undoing the changes on that PR buh it wasn't working the way i expected it to.

buh can we test this now ? to see if it fixes the race issues...

@daveads
Copy link
Contributor Author

daveads commented Oct 3, 2024

@alixander reverting back undo the changes i recently made... and revert back to what was already there...

is this what you wanted ?

it doesn't keep my current changes that fix the race issue.

@alixander
Copy link
Contributor

alixander commented Oct 5, 2024

i've reverted the previous fix that didn't work in master, please rebase

@daveads
Copy link
Contributor Author

daveads commented Oct 6, 2024

@alixander it's ready for merge.

Copy link
Contributor

@alixander alixander left a comment

Choose a reason for hiding this comment

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

There's some miscommunication here.

Your previous PR (#19) did not work. So it looks like your second attempt is trying a new, completely different strategy. So I wanted the previous one reverted. And now you've just included both in this PR. I have said the same thing in an earlier comment #23 (comment)

Please also stop pinging me repeatedly, just request review if needed. Your behavior in general is strange, with the closing and re-opening issues, attempting to revert and undo the revert of your last PR. This pattern is frustrating and if you continue down this line, I will no longer be accepting your contributions.

@daveads
Copy link
Contributor Author

daveads commented Oct 7, 2024

There's some miscommunication here.

Your previous PR (#19) did not work. So it looks like your second attempt is trying a new, completely different strategy. So I wanted the previous one reverted. And now you've just included both in this PR. I have said the same thing in an earlier comment #23 (comment)

Please also stop pinging me repeatedly, just request review if needed. Your behavior in general is strange, with the closing and re-opening issues, attempting to revert and undo the revert of your last PR. This pattern is frustrating and if you continue down this line, I will no longer be accepting your contributions.

Okay, closing and reopening the PR was an attempt to resolve an issue.
And sure, I'll stop pinging—I didn’t realize it was considered unusual. I tend to do that often.

@daveads
Copy link
Contributor Author

daveads commented Oct 7, 2024

Git rebase is something I don't particularly like messing with, but I hope this does what you needed.

@alixander
Copy link
Contributor

Thank you for your attempt @daveads but you're not understanding me and I've tried many times now, I'm no longer accepting your contributions.

@alixander alixander closed this Oct 7, 2024
@daveads daveads deleted the fix branch October 7, 2024 17:05
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.

2 participants