Skip to content

Follow-ups from #803: overlapping Shutdown calls share the shortest budget, the Engine.Shutdown doc sentence, a test margin nit #821

Description

@FumingPower3925

Follow-ups from the round-1 review of #803 (celeris#753), none blocking.

  1. Overlapping Shutdown calls: the shortest budget ends the shared drain for every caller. Each caller's AfterFunc cancels drainCtx (engine/std/engine.go at 325d83a, about line 246). A caller whose own ctx is still live then gets the internal drainCtx's context.Canceled from http.Server.Shutdown(drainCtx) (the return err at about line 260), since ctx.Err() is nil for it. Before fix(std): a cancel of StartWithContext's context keeps Config.ShutdownTimeout (celeris#753) #803 the winner's budget governed. Unlikely call pattern, misleading error: return nil (or a documented error) to a caller whose own ctx has not expired, or say so in the Shutdown doc. Static reading; not run.
  2. engine.Engine.Shutdown's doc at fix(std): a cancel of StartWithContext's context keeps Config.ShutdownTimeout (celeris#753) #803's head says "No engine closes a connection whose handler is still running when ctx expires; the handler runs to completion". That is false on epoll, io_uring and adaptive for an HTTP/2 stream on the shared worker pool (celeris#759). fix(epoll, iouring, std): graceful shutdown waits for the HTTP/2 streams on the shared worker pool, and std for its h2c streams (celeris#759) #808 (which carries fix(std): a cancel of StartWithContext's context keeps Config.ShutdownTimeout (celeris#753) #803) rewrites the doc; if fix(std): a cancel of StartWithContext's context keeps Config.ShutdownTimeout (celeris#753) #803 merges alone, the sentence stays wrong until fix(epoll, iouring, std): graceful shutdown waits for the HTTP/2 streams on the shared worker pool, and std for its h2c streams (celeris#759) #808 does.
  3. Nit: el < budget in engine/std/listen_cancel_budget_test.go (about lines 133-141) has a sub-microsecond theoretical margin: shutCtx is created before start is taken. Creating the context after start removes it.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    area/engineEngine interface or implementationbugSomething isn't working

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions