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

[FEATURE REQUEST] Customization of behavior when rr is unable to allocate worker #471

Closed
jjsaunier opened this issue Jan 9, 2021 · 6 comments · Fixed by #472 or roadrunner-server/roadrunner-docs#37
Assignees
Labels
C-feature-request Category: feature requested, but need to be discussed
Milestone

Comments

@jjsaunier
Copy link

jjsaunier commented Jan 9, 2021

👋 @48d90782

follow up after discord discussion

Goal : Distinct error from worker allocation to be able to retry on another rr instance at load balancer level.

Current State : When rr is not able to allocate a worker, it's send a 500, which is not the best option because application that rr serve could also send 500 HTTP code.

IHMO, rr should give the possibilty to the user to:

With this 3 possibilities I think we good, if you see / have another use case, don't hesitate

https://github.com/spiral/roadrunner/blob/5dc83ef5eee77f4d1ef557e5f8b566e75892680d/service/http/handler.go#L134-L151

@rustatian rustatian changed the title Customization of behavior when rr is unable to allocate worker [FEATURE REQUEST] Customization of behavior when rr is unable to allocate worker Jan 9, 2021
@rustatian rustatian added the C-feature-request Category: feature requested, but need to be discussed label Jan 9, 2021
@rustatian rustatian self-assigned this Jan 9, 2021
@rustatian rustatian modified the milestones: 2.0.0, unplanned Jan 9, 2021
@rustatian
Copy link
Member

👋 @jjsaunier
I quite agree with your proposal. I'll think about implementation because there is not only an "allocate worker" error that can be returned from the pool.Exec method. There are also fmt.Errorf("payload can not be empty"), fmt.Errorf("worker is not ready (%s)", w.state.String()) and may be more.
I think that it is better to distinct all RR errors from the errors which come from the PHP.
As for the fast solution, that you can do on your own - is to temporarily replace w.WriteHeader(500) with w.WriteHeader(502) or 504 and then build docker image (docker build -t roadrunner:local -f Dockerfile .) or invoke make build target in the Makefile.

@rustatian rustatian modified the milestones: unplanned, 1.9.2 Jan 9, 2021
@jjsaunier
Copy link
Author

Indeed, i'm only experiencing the issue with allocate worker, so no experience on payload can not be empty, worker is not ready but sounds good to dissociate all rr error vs app error 👍

Thanks for the quick solution, unfortunately traefik (which I'm using) does not support retry based on status code at this time... (I hope a day it will!)

So currently my trick to let traefik retrying, I close the connection without responding :

hj, ok := w.(http.Hijacker)

if !ok {
	http.Error(w, "", http.StatusGatewayTimeout)
	return
}
		
conn, _, err := hj.Hijack()
conn.Close()

@rustatian
Copy link
Member

@jjsaunier #472
1.9.2 milestone

@jjsaunier
Copy link
Author

jjsaunier commented Jan 12, 2021

You rocks @48d90782! 🥇

I left comment on pr

@bors bors bot closed this as completed in f18e7f6 Jan 13, 2021
@rustatian
Copy link
Member

@wolfy-j
Copy link
Contributor

wolfy-j commented Feb 23, 2021

I believe we will need to add the same thing for RR2 down the road.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: feature requested, but need to be discussed
Projects
None yet
3 participants