Skip to content

refactor: removes context on the C side - #1404

Merged
dunglas merged 23 commits into
mainfrom
feat/auto-scale-clock-time-fc-better-timeouts-refactor-context
Mar 10, 2025
Merged

refactor: removes context on the C side#1404
dunglas merged 23 commits into
mainfrom
feat/auto-scale-clock-time-fc-better-timeouts-refactor-context

Conversation

@AlliBalliBaba

@AlliBalliBabaAlliBalliBaba commented Feb 22, 2025•
edited
Loading

Copy link
Copy Markdown
Contributor

Most of this PR is a refactor of the FrankenPHPContext. I noticed that having a context on both the C and the go side can be pretty confusing and error prone. Especially considering that fatal errors (like timeouts) will make a worker just jump to the end of a script. This PR removes most of the context on the C side and decouples some components from net/http by passing the context in channels.
Having minimal thread-local context on the C side should also make it easier to execute PHP directly in go threads in the future.

@AlliBalliBabaAlliBalliBaba changed the title Feat: adds busy timeoutsrefactor: removes contet on the C sideFeb 23, 2025
@AlliBalliBabaAlliBalliBaba changed the title refactor: removes contet on the C siderefactor: removes context on the C sideFeb 23, 2025
@AlliBalliBaba
AlliBalliBaba marked this pull request as ready for review February 23, 2025 23:35
dunglas
dunglas reviewed Feb 24, 2025
View reviewed changes
Comment threadcaddy/caddy.go Outdated
@Reinhard-Berger

Reinhard-Berger commented Feb 26, 2025

Copy link
Copy Markdown

Will this impact Performance?

dunglas
dunglas requested changes Feb 26, 2025
View reviewed changes
Comment threadcontext.go Outdated
Comment threadcontext.go Outdated
Comment threadcontext.go Outdated
Comment threadcontext.go
Comment threadcontext.go
Comment threadfrankenphp.c
Comment threadfrankenphp.c
Comment threadfrankenphp.c
Comment threadcontext.go
@AlliBalliBaba

AlliBalliBaba commented Feb 26, 2025

Copy link
Copy Markdown
ContributorAuthor

@Reinhard-Berger this will not affect performance

@AlliBalliBaba

AlliBalliBaba commented Mar 7, 2025

Copy link
Copy Markdown
ContributorAuthor

@dunglas let me know once you're ready to merge this, I have some small follow-ups that this PR enables.

@AlliBalliBaba

AlliBalliBaba commented Mar 8, 2025

Copy link
Copy Markdown
ContributorAuthor

We can also wait for #1376, pretty sure there will be some small conflicts.

@withinboredom

withinboredom commented Mar 9, 2025

Copy link
Copy Markdown
Member

I have some small follow-ups that this PR enables.

Yes. Slow down on the major refactors :p

I have a few too, but just been waiting on things to stabilize. Seriously good work though!

@dunglas
dunglas merged commit f50248a into mainMar 10, 2025
@dunglas
dunglas deleted the feat/auto-scale-clock-time-fc-better-timeouts-refactor-context branch March 10, 2025 07:44
@dunglas

dunglas commented Mar 10, 2025

Copy link
Copy Markdown
Member

Great!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dunglasdunglasdunglas approved these changes
@withinboredomwithinboredomwithinboredom approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@AlliBalliBaba@Reinhard-Berger@withinboredom@dunglas