Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions cli/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ type Settings struct {
Debug bool `json:"debug"`
AcID string `json:"acId"`
Campus bool `json:"campusOnly"`
Timeout int `json:"timeout"`
}

var logger = loggo.GetLogger("auth-thu")
Expand Down Expand Up @@ -99,7 +100,14 @@ func mergeCliSettings(c *cli.Command) {
merged.AcID = settings.AcID
}
merged.Campus = settings.Campus || c.Bool("campus-only")
merged.Timeout = c.Int("timeout")
if !c.IsSet("timeout") && settings.Timeout != 0 {
merged.Timeout = settings.Timeout
}
settings = merged
if settings.Timeout > 0 {
Comment on lines +103 to +108

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个感觉有点意义不明,命令行参数允许--timeout 0,但是配置文件的timeout: 0是无效的会被忽略,感觉这样才更迷惑了。

其实完全可以不改,explicit "--timeout 0" will be silently ignored本来也不算什么大问题

libauth.HttpTimeout = time.Duration(settings.Timeout) * time.Second
}
logger.Debugf("Settings Username: \"%s\"\n", settings.Username)
logger.Debugf("Settings Ip: \"%s\"\n", settings.Ip)
logger.Debugf("Settings Host: \"%s\"\n", settings.Host)
Expand All @@ -114,6 +122,7 @@ func mergeCliSettings(c *cli.Command) {
logger.Debugf("Settings Debug: %t\n", settings.Debug)
logger.Debugf("Settings AcID: \"%s\"\n", settings.AcID)
logger.Debugf("Settings Campus: %t\n", settings.Campus)
logger.Debugf("Settings Timeout: %d\n", settings.Timeout)
}

func requestUser() (err error) {
Expand Down Expand Up @@ -424,6 +433,7 @@ func main() {
&cli.StringFlag{Name: "config-file", Aliases: []string{"c"}, Usage: "`path` to your config file, default ~/.auth-thu"},
&cli.StringFlag{Name: "hook-success", Usage: "command line to be executed in shell after successful login/out"},
&cli.IntFlag{Name: "online-interval", Aliases: []string{"I"}, Usage: "the interval between each keepAlive request (s)", Value: 3},
&cli.IntFlag{Name: "timeout", Aliases: []string{"t"}, Usage: "HTTP request timeout in seconds for the auth server", Value: 30},

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This default should be change to 2s as well?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

关于把超时默认值从 30s 调回 2s 这个改动,其实我个人感觉:

  • timeout 是上限,不是固定耗时。设成 30s 不代表请求要花 30s —— 网络好的时候照样几毫秒返回,对正常用户的体验没有任何影响。

  • 2s 在部分场景下确实会误伤。issue #26 里就有反馈说 FIT 楼里 auth server 响应能到 ~16s,2s 直接就 timeout 失败了。把上限抬高只是提高容错,并不会拖慢正常路径。

  • 对网络敏感、希望快速失败重试的用户可以自己显式调成 2s,但如果默认值面向的是"大多数用户在大多数网络下能正常用",这种场景下 30s 是更安全的兜底。

@jiegec

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

不过已经在 960882b 里改回 2s。

&cli.BoolFlag{Name: "daemonize", Aliases: []string{"D"}, Usage: "run without reading username/password from standard input; less log"},
&cli.BoolFlag{Name: "debug", Usage: "print debug messages"},
&cli.BoolFlag{Name: "help, h", Usage: "print the help"},
Expand Down
10 changes: 7 additions & 3 deletions libauth/requests.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ import (

var logger = loggo.GetLogger("libauth")

// HttpTimeout is the timeout used for all HTTP requests issued by libauth.
// It can be overridden by the caller (e.g. the CLI) before making requests.
var HttpTimeout = 30 * time.Second
Comment thread
WangYihang marked this conversation as resolved.
Outdated

func extractJSONFromJSONP(jsonp, callbackName string) (string, error) {
l := len(callbackName)
if len(jsonp) < l+2 {
Expand Down Expand Up @@ -90,7 +94,7 @@ func GetJSON(baseUrl string, params url.Values) (string, error) {
const CB = "C_a_l_l_b_a_c_k"
params.Set("callback", CB)
var netClient = &http.Client{
Timeout: time.Second * 2,
Timeout: HttpTimeout,
}
url := baseUrl + "?" + params.Encode()
logger.Debugf("GET \"%s\"\n", url)
Expand All @@ -109,7 +113,7 @@ func GetJSON(baseUrl string, params url.Values) (string, error) {
func IsOnline(host *UrlProvider, acID string) (online bool, err error, username string) {
logger.Debugf("Check if online\n")
var netClient = &http.Client{
Timeout: time.Second * 2,
Timeout: HttpTimeout,
}
online = false
params := url.Values{
Expand Down Expand Up @@ -174,7 +178,7 @@ func GetNasID(IP, user, password string) (nasID string, err error) {
func GetAcID(V6 bool) (acID string, err error) {
logger.Debugf("Get AC ID\n")
var netClient = &http.Client{
Timeout: time.Second * 2,
Timeout: HttpTimeout,
CheckRedirect: func(req *http.Request, via []*http.Request) error {
logger.Debugf("REDIRECT \"%v\"\n", req.URL)
return errors.New("should not redirect")
Expand Down
Loading