Skip to content

fix(server): return nil from Run() retry path instead of misleading error#311

Closed
wc4440222 wants to merge 1 commit into
ccfos:mainfrom
wc4440222:fix/server-run-retry-error
Closed

fix(server): return nil from Run() retry path instead of misleading error#311
wc4440222 wants to merge 1 commit into
ccfos:mainfrom
wc4440222:fix/server-run-retry-error

Conversation

@wc4440222

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes a misleading error return in internal/server/server.go Run() method.

Problem

When retry is enabled (RetryMaxTime > 0 && RetryInterval > 0), Run() launches the server in a background goroutine with exponential backoff but always returns fmt.Errorf("init err") to the caller. This misleads callers into thinking the server failed to start when it is actually running.

// Before
func (s *server) Run(option *Option) error {
    if option.RetryMaxTime > 0 && option.RetryInterval > 0 {
        go func() {
            // ... actual retry logic works fine ...
        }()
        return fmt.Errorf("init err")  // ← lying to the caller
    }
    return s.run(option.Addr)
}

The caller already uses _ = s.Run(...) which masks the error, so this bug has gone unnoticed.

Fix

Return nil from the retry path since the server IS starting. Also log the retry configuration at startup for visibility.

// After
func (s *server) Run(option *Option) error {
    if option.RetryMaxTime > 0 && option.RetryInterval > 0 {
        log.Infof("tcp api server starting with retry on %s (max=%v, interval=%v)", ...)
        go func() { /* ... retry logic unchanged ... */ }()
        return nil  // ← server is starting, no error
    }
    return s.run(option.Addr)
}

Testing

  • go build ./internal/server/ — compiles ✅
  • go test ./internal/server/ -count=1 — all 21 existing tests pass ✅

Checklist

  • No breaking API changes
  • All existing tests pass
  • DCO signed

Signed-off-by: wc4440222 [email protected]

@huhong-web

Copy link
Copy Markdown
Contributor

Code Review Result — lgtm ✅

1. Copyright Check

  • ✅ No new files — existing file modified

2. Go Style

  • ✅ Replaced misleading fmt.Errorf("init err") with correct return nil
  • ✅ Added informative log message for retry configuration
  • ✅ Improved function documentation

3. Abstract Interface Check

  • ✅ N/A — bug fix only

4. Architecture Check

  • ✅ Fix eliminates lying to callers about server state
  • ✅ Retry logic unchanged — minimal diff

5. Concurrency Safety

  • ✅ N/A — goroutine launch pattern unchanged

6. Security

  • ✅ No security concerns

Verdict: lgtm — Clean fix for misleading error return with improved observability.

…rror

When retry is enabled, Run() launches the server in a background
goroutine but previously returned fmt.Errorf("init err") which
misled callers into thinking the server failed to start. Return nil
instead since the server is actually starting in the background.

Also log the retry configuration at startup for visibility.

Signed-off-by: wc4440222 <[email protected]>
@wc4440222
wc4440222 force-pushed the fix/server-run-retry-error branch from 7ec3891 to f83ba97 Compare July 7, 2026 08:06
@wc4440222

Copy link
Copy Markdown
Contributor Author

@hao022 @huhong-web Hi, this PR has been lgtm'd for 9 days. Could you please take a look and merge when convenient? Thanks!

@hao022

hao022 commented Jul 24, 2026

Copy link
Copy Markdown
Member

@wc4440222 你好感谢贡献代码。当前已经重构了server 模块。建议删除run 这个函数,实现和huatuo-apiserver 相同的调用方式。

推荐修复

删除旧的 Run 和 Option,让 huatuo-bamai 也使用:

Start → Done/Wait → Shutdown

具体调整:

  1. cmd/huatuo-bamai/handlers.Start 返回 (RunningServer, error)。
  2. 使用 httpServer.Start(opts.Addr)。
  3. startHandlers 保存返回的 server。
  4. 将 server.Shutdown 注册为 Daemon cleanup。
  5. Daemon 监听 server.Done(),处理服务异常退出。
  6. 如果端口占用确实需要重试,在调用 Start 的启动阶段做 context-aware 重试。

重试逻辑应类似:

  for {
  	err := httpServer.Start(addr)
  	if err == nil {
  		return httpServer, nil
  	}

  	timer := time.NewTimer(backoffDuration)
  	select {
  	case <-ctx.Done():
  		timer.Stop()
  		return nil, fmt.Errorf("start HTTP server: %w", ctx.Err())
  	case <-timer.C:
  	}
  }

@wc4440222

Copy link
Copy Markdown
Contributor Author

Closing this PR and resubmitting as #413 per @hao022 feedback — removed Run/Option and switched to Start/Done/Wait/Shutdown pattern matching huatuo-apiserver.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants