Skip to content

refactor: cleanup modules + c#1955

Merged
AlliBalliBaba merged 17 commits intomainfrom
refator/cleanup-c
Dec 2, 2025
Merged

refactor: cleanup modules + c#1955
AlliBalliBaba merged 17 commits intomainfrom
refator/cleanup-c

Conversation

@AlliBalliBaba
Copy link
Contributor

@AlliBalliBaba AlliBalliBaba commented Nov 1, 2025

This refactor moves state.go and backoff.go to their own internal modules, freeing up the main module.

It also cleans up some unused C structs/functions and changes it so the thread-local is_worker_thread gets set once before going ready, instead of on each request_info update.

@AlliBalliBaba AlliBalliBaba changed the title Refator/cleanup c refactor: cleanup modules + c Nov 1, 2025
@AlliBalliBaba
Copy link
Contributor Author

Very strange, still seeing random build errors with internal module tests. Maybe this is due to some kind of caching in the pipeline?

@AlliBalliBaba
Copy link
Contributor Author

AlliBalliBaba commented Nov 10, 2025

It looks like internal test compilation will sometimes fail since the modules are not aware that we are using CGO, but just importing C in the modules fixes compilation.

Something to be aware of.

@AlliBalliBaba AlliBalliBaba marked this pull request as ready for review November 10, 2025 19:28
"time"
)

type ExponentialBackoff struct {
Copy link
Member

Choose a reason for hiding this comment

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

Would it have any benefit to rely on a library such as https://pkg.go.dev/github.com/cenkalti/backoff/v5 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably not, this little code is likely easier to maintain than the dependency.

TBH we could probably just remove the backoff and return an error immediately if startup fails. In the past, it would also fail on restart but that logic has since been removed (after complaints).
Workers randomly (and expectedly?) failing on startup is probably something that should be handled by a supervisor, not by us.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see #1970 for the solution without backoff

@AlliBalliBaba
Copy link
Contributor Author

AlliBalliBaba commented Nov 11, 2025

Apparently the mac os runners are getting deprecated actions/runner-images#13046

@henderkes
Copy link
Contributor

They already tend to never pick up queued jobs. I've created a PR in the homebrew repo, but forgot about our CI here.

Should be enough to change macos-13 to macos-15-intel.

AlliBalliBaba and others added 4 commits November 13, 2025 23:38
* removes backoff.

* Adjusts comment.

* Suggestions by @dunglas

* Removes 'max_consecutive_failures'

* Removes 'max_consecutive_failures'

* Adjusts warning.

* Disables the logger in tests.

* Revert "Adjusts warning."

This reverts commit e93a6a9.

* Revert "Removes 'max_consecutive_failures'"

This reverts commit ba28ea0.

* Revert "Removes 'max_consecutive_failures'"

This reverts commit 32e649c.

* Only fails on max failures again.

* Restores failure timings.
# Conflicts:
#	phpmainthread_test.go
# Conflicts:
#	phpthread.go
#	threadinactive.go
#	threadregular.go
#	threadworker.go
#	worker.go
# Conflicts:
#	phpmainthread_test.go
#	threadregular.go
@henderkes
Copy link
Contributor

Ignore static binary failures, spc PR is already open.

@AlliBalliBaba AlliBalliBaba merged commit 98573ed into main Dec 2, 2025
133 of 142 checks passed
@AlliBalliBaba AlliBalliBaba deleted the refator/cleanup-c branch December 2, 2025 22:10
@AlliBalliBaba
Copy link
Contributor Author

This one is gonna create a few merge conflicts 😅

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

Comments