独立服务http超时不生效 #3460
独立服务http超时不生效 #3460
Conversation
dosubot
bot
added
the
size:M
|
This PR looks strange, exposing the underlying implementation, but not completely |
大佬的意思是，将其他的参数也尽可能实现了吗 |
|
私信问了下@shenqidebaozi。听他的意思说是应该暴露一个直接修改 Server 的配置函数。假设 net/http.Server 有 10 个可访问字段，现在加了 2 个，是不是以后就要再加 8 个？而且这是“跨级”的配置，改的不是 kratos/http.Server 的字段，而是 kratos/http.Server.Server.xxx。可能上文的“strange”指的是这种跨级配置的方式 strange 吧。 那按照这个思路来的话还是有点……emmm，看代码吧： // 比如这样
func WithServer(srv *http.Server) ServerOption {
return func(s *Server) {
s.Server = srv
}
}
// 用起来大概这样
srv := &net.http.Server{...}
s := http.NewServer(http.WithServer(srv))
// 但是这样写的话会有问题
// 就拿 NewServer 里的逻辑来说吧
srv.Server = &http.Server{
Handler: FilterChain(srv.filters...)(srv.router),
TLSConfig: srv.tlsConf,
}
// 要是用户指定了 Handler 或 TLSConfig 的话我们自己的配置就被覆盖了。
// 要么就是用户的配置被覆盖，一样是 bug。
// 再就是 327 行的 BaseContext 也是后指定的，如果用户覆盖这个字段就会导致问题。
// 其实现在 server 是一个可访问字段，现在直接这么改也行。
s := http.NewServer(http.Address(":8080"))
s.Server.ReadTimeout = time.Second
s.Server.WriteTimeout = time.Second
s.Start(ctx) |
I sent a private message asking about the package. It sounds like he means that a configuration function that directly modifies the Server should be exposed. Suppose net/http.Server has 10 accessible fields, and now 2 have been added, will 8 more be added in the future? And this is a "cross-level" configuration. What is changed is not the field of kratos/http.Server, but kratos/http.Server.Server.xxx. Maybe the "strange" above refers to this cross-level configuration strange. If you follow this idea, it's still a bit...emmm, let's look at the code: // Like this
func WithServer(srv *http.Server) ServerOption {
return func(s *Server) {
s.Server = srv
}
}
// It looks like this
srv := &net.http.Server{...}
s := http.NewServer(http.WithServer(srv))
// But there will be problems if you write it like this
// Let’s take the logic in NewServer as an example
srv.Server = &http.Server{
Handler: FilterChain(srv.filters...)(srv.router),
TLSConfig: srv.tlsConf,
}
// If the user specifies Handler or TLSConfig, our own configuration will be overwritten.
// Either the user's configuration has been overwritten, which is also a bug.
// Then the BaseContext on line 327 is also specified later. If the user overwrites this field, it will cause problems.
//In fact, server is an accessible field now, so you can just change it like this now.
s := http.NewServer(http.Address(":8080"))
s.Server.ReadTimeout = time.Second
s.Server.WriteTimeout = time.Second
s.Start(ctx) |
好的，感谢解答，那我关了吧 |
Okay, thanks for the answer, I'll turn it off then. |
|
有点可惜了，其实我觉得这改动还行的【叹气】 |
It's a pity, actually I think this change is okay [sigh] |
Hahaha It’s okay, it’s just that there was a problem when I tested it. Also, I find your explanation above very convincing. If there are any problems in the future, I will continue to submit PRs. |
哈哈哈 没事的，只是我用的时候测试出来问题。另外，您上面的解释我感觉很有说服力。后续有问题我持续提PR |
Hahaha It’s okay, it’s just that there was a problem when I tested it. Also, I find your explanation above very convincing. If there are any problems in the future, I will continue to submit PRs. |
Description (what this PR does / why we need it):
Which issue(s) this PR fixes (resolves / be part of):
#3412
Other special notes for the reviewers: