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

Support Server-Sent-Events? #2029

Closed
icecoffee531 opened this issue Jun 17, 2022 · 17 comments · Fixed by #2041
Closed

Support Server-Sent-Events? #2029

icecoffee531 opened this issue Jun 17, 2022 · 17 comments · Fixed by #2041
Labels

Comments

@icecoffee531
Copy link

Is your feature request related to a problem? Please describe.
Go-zero unsupport SSE without set global config Timeout to 0. When i want to use SSE, must to set Header {"upgrade": "websocket"}. But chrome unsupport change header Upgrade

Describe the solution you'd like
A clear and concise description of what you want to happen.
Add a new header to identify the request SSE.

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.
Add new config for SSE. If set SSE config, skip timeoutHandler

Additional context
Add any other context or screenshots about the feature request here.
scheme 1
PR: #2028

scheme 2
image

@kevwan
Copy link
Contributor

kevwan commented Jun 17, 2022

the request header is Accept: text/event-stream?

Sse: true is not standard, right?

@icecoffee531
Copy link
Author

icecoffee531 commented Jun 17, 2022

the request header is Accept: text/event-stream?

@kevwan Yes, i have set the header Accept: text/event-stream.

Sse: true is not standard, right?

@kevwan Right, the header is customized not standard.

@kevwan
Copy link
Contributor

kevwan commented Jun 17, 2022

So shall we check the header like this?

@icecoffee531
Copy link
Author

icecoffee531 commented Jun 17, 2022

So shall we check the header like this?

I don't think this is the best solution. But SSE is not like websocket , it doesn't have its own header. So I customized a header, if you have a better solution that would be great.

@chenquan
Copy link
Member

Maybe can let the user to specify ignore route timeout?

go-zero/rest/engine.go

Lines 131 to 137 in 86d7031

func (ng *engine) checkedTimeout(timeout time.Duration) time.Duration {
if timeout > 0 {
return timeout
}
return time.Duration(ng.conf.Timeout) * time.Millisecond
}

replace:

func (ng *engine) checkedTimeout(timeout time.Duration, ignoreTimeout bool) time.Duration {
	if ignoreTimeout {
		// ignore timeout
		return -1
	}

	if timeout > 0 {
		return timeout
	}

	return time.Duration(ng.conf.Timeout) * time.Millisecond
}

go-zero/rest/types.go

Lines 33 to 40 in 86d7031

featuredRoutes struct {
timeout time.Duration
priority bool
jwt jwtSetting
signature signatureSetting
routes []Route
maxBytes int64
}

replace:

featuredRoutes struct {
	timeout       time.Duration
	priority      bool
	jwt           jwtSetting
	signature     signatureSetting
	routes        []Route
	maxBytes      int64
	ignoreTimeout bool
}

// WithIgnoreTimeout returns a RunOption to ignore the timeout.
func WithIgnoreTimeout() RouteOption {
	return func(r *featuredRoutes) {
		r.ignoreTimeout = true
	}
}

@icecoffee531
Copy link
Author

icecoffee531 commented Jun 18, 2022

Maybe can let the user to specify ignore route timeout?

go-zero/rest/engine.go

Lines 131 to 137 in 86d7031

func (ng *engine) checkedTimeout(timeout time.Duration) time.Duration {
if timeout > 0 {
return timeout
}
return time.Duration(ng.conf.Timeout) * time.Millisecond
}

replace:

func (ng *engine) checkedTimeout(timeout time.Duration, ignoreTimeout bool) time.Duration {
	if ignoreTimeout {
		// ignore timeout
		return -1
	}

	if timeout > 0 {
		return timeout
	}

	return time.Duration(ng.conf.Timeout) * time.Millisecond
}

go-zero/rest/types.go

Lines 33 to 40 in 86d7031

featuredRoutes struct {
timeout time.Duration
priority bool
jwt jwtSetting
signature signatureSetting
routes []Route
maxBytes int64
}

replace:

featuredRoutes struct {
	timeout       time.Duration
	priority      bool
	jwt           jwtSetting
	signature     signatureSetting
	routes        []Route
	maxBytes      int64
	ignoreTimeout bool
}
// WithIgnoreTimeout returns a RunOption to ignore the timeout.
func WithIgnoreTimeout() RouteOption {
	return func(r *featuredRoutes) {
		r.ignoreTimeout = true
	}
}

Pretty good.

@kevwan
Copy link
Contributor

kevwan commented Jun 21, 2022

Accept: text/event-stream

the request header is Accept: text/event-stream?

@kevwan Yes, i have set the header Accept: text/event-stream.

Sse: true is not standard, right?

@kevwan Right, the header is customized not standard.

I think this is the header to check.

@github-actions
Copy link

This issue is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale label Jul 22, 2022
@github-actions
Copy link

github-actions bot commented Aug 6, 2022

This issue was closed because it has been inactive for 14 days since being marked as stale.

@github-actions github-actions bot closed this as completed Aug 6, 2022
@lxkaka
Copy link

lxkaka commented May 12, 2023

the issue is still exist

@waltcow
Copy link

waltcow commented Jun 29, 2023

issue is still exist @kevwan

@i255979
Copy link

i255979 commented Jul 15, 2023

issue is still exist @kevwan
http api config

Name: client-api
Host: 0.0.0.0
Port: 8101
Timeout: 60000
Middlewares:
Timeout: false <---set timeout false, would be resolve
JwtAuth:
AccessSecret: "12345678"
AccessExpire: 9999999

@lxkaka
Copy link

lxkaka commented Jul 17, 2023

@i255979 这样是可以绕过;但是有个问题所有的接口都没有超时设置了

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


@i255979 This can be bypassed; but there is a problem that all interfaces have no timeout settings

@wuqinqiang
Copy link
Contributor

Createing a new route can resolve this issue,

demo:

server.AddRoute(rest.Route{
		Method:  http.MethodGet,
		Path:    "xx",
		Handler: lottery.CurrentLotteryStateEventHandler(ctx),
	}, rest.WithTimeout(xxx)) 

only this route uses this timeout, other routers timeout from conf.

@lxkaka
Copy link

lxkaka commented Mar 7, 2024

To add to the previous comment
we can define in api file like follow:

@server (
    prefix: /v1
    group: Login
    timeout: 30s
)
service user {
    @handler getUserInfo
    get /user/info/:id (GetUserInfoReq) returns (GetUserInfoResp)
}

@chenquan
Copy link
Member

chenquan commented Mar 7, 2024

#2041

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
8 participants