go-kratos / kratos

Your ultimate Go microservices framework for the cloud-native era.
https://go-kratos.dev
MIT License
23.43k stars 4.01k forks source link

独立服务http超时不生效 #3460

Closed smilefisher closed 3 weeks ago

smilefisher commented 3 weeks ago

Description (what this PR does / why we need it):

Which issue(s) this PR fixes (resolves / be part of):

https://github.com/go-kratos/kratos/issues/3412

Other special notes for the reviewers:

shenqidebaozi commented 3 weeks ago

This PR looks strange, exposing the underlying implementation, but not completely

smilefisher commented 3 weeks ago

This PR looks strange, exposing the underlying implementation, but not completely

大佬的意思是,将其他的参数也尽可能实现了吗

kvii commented 3 weeks ago

私信问了下@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)
kratos-ci-bot commented 3 weeks ago

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


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)
smilefisher commented 3 weeks ago
// 再就是 327 行的 BaseContext 也是后指定的,如果用户覆盖这个字段就会导致问题。

好的,感谢解答,那我关了吧

kratos-ci-bot commented 3 weeks ago

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


// Then the BaseContext on line 327 is also specified later. If the user overwrites this field, it will cause problems.

Okay, thanks for the answer, I'll turn it off then.

kvii commented 3 weeks ago

有点可惜了,其实我觉得这改动还行的【叹气】

kratos-ci-bot commented 3 weeks ago

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


It's a pity, actually I think this change is okay [sigh]

kratos-ci-bot commented 3 weeks ago

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


// Then the BaseContext on line 327 is also specified later. If the user overwrites this field, it will cause problems.

Okay, thanks for the answer, I'll turn it off then.

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.

smilefisher commented 3 weeks ago

有点可惜了,其实我觉得这改动还行的【叹气】

哈哈哈 没事的,只是我用的时候测试出来问题。另外,您上面的解释我感觉很有说服力。后续有问题我持续提PR

kratos-ci-bot commented 3 weeks ago

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


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.