Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

x/sys/windows/svc: goroutine leak on exit path #35953

Closed
zhangyoufu opened this issue Dec 4, 2019 · 3 comments
Closed

x/sys/windows/svc: goroutine leak on exit path #35953

zhangyoufu opened this issue Dec 4, 2019 · 3 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows
Milestone
Unreleased

Comments

@zhangyoufu
Copy link
Contributor

https://github.com/golang/sys/blob/6d18c012aee9febd81bbf9806760c8c4480e870d/windows/svc/service.go#L248-L279

loop:
	for {
		select {
		case r := <-inch:
			if r.errno != 0 {
				ec.errno = r.errno
				break loop
			}
			inch = nil
			outch = cmdsToHandler
			outcr.Cmd = r.cmd
			outcr.EventType = r.eventType
			outcr.EventData = r.eventData
			outcr.Context = r.context
		case outch <- outcr:
			inch = s.c
			outch = nil
		case c := <-changesFromHandler:
			err := s.updateStatus(&c, &ec)
			if err != nil {
				// best suitable error number
				ec.errno = sysErrSetServiceStatusFailed
				if err2, ok := err.(syscall.Errno); ok {
					ec.errno = uint32(err2)
				}
				break loop
			}
			outcr.CurrentStatus = c
		case ec = <-exitFromHandler:
			break loop
		}
	}

If the handler send to changesFromHandler channel after the break loop due to r.errno != 0, the handler goroutine and the unbuffered channel will be leaked.

I understand that sysErrNewThreadInCallback can hardly happen, and it is generally safe to assume that caller will exit after svc.Run returned. This is just a minor issue.

Some other places that caught my eyes:

  1. svc.Run called os.LockOSThread(), but does not have defer os.UnlockOSThread()
  2. service.close() is not referenced anywhere, and it should be removed because CloseHandle while WaitForSingleObject is an undefined behavior
@gopherbot gopherbot added this to the Unreleased milestone Dec 4, 2019
@ianlancetaylor
Copy link
Contributor

This seems like a problem with the Handler interface. It has no support for shutting down the goroutine. I don't know how important this is.

CC @alexbrainman

@ianlancetaylor ianlancetaylor added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows labels Dec 4, 2019
@alexbrainman
Copy link
Member

If the handler send to changesFromHandler channel after the break loop due to r.errno != 0, the handler goroutine and the unbuffered channel will be leaked.

I understand that sysErrNewThreadInCallback can hardly happen, and it is generally safe to assume that caller will exit after svc.Run returned. This is just a minor issue.

If r.errno != 0, then service is exiting, and it doesn't matter what happens to leaked handler goroutine or unbuffered channel.

  1. svc.Run called os.LockOSThread(), but does not have defer os.UnlockOSThread()

Same. When StartServiceCtrlDispatcher returns, it means that service is about to exit. So it does not matter what happens to goroutine that calls os.LockOSThread().

2. service.close() is not referenced anywhere, and it should be removed because CloseHandle while WaitForSingleObject is an undefined behavior

Yes, service.close() is not called by anything. I don't remember why it is there. Perhaps it was used for debugging during development. I would rather leave it as is. It is just a couple lines of code. If you not insist.

Thank you.

Alex

@zhangyoufu
Copy link
Contributor Author

Thank you for explanation. I'm fine with current implementation.

If anyone want to implement multi-service shared hosting, this issue might be helpful.

@golang golang locked and limited conversation to collaborators Dec 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@zhangyoufu @ianlancetaylor @gopherbot @alexbrainman