-
Notifications
You must be signed in to change notification settings - Fork 180
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.
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
Added runtime.KeepAlive for the new stricter 1.8 GC #48
Conversation
file.go
Outdated
// code to ioCompletionProcessor, c and bytes must both remain alive | ||
// until the channel read is complete. | ||
runtime.KeepAlive(c) | ||
runtime.KeepAlive(bytes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bytes [](start = 20, length = 5)
I don't think we need to do anything with bytes here (it's not even a pointer).
file.go
Outdated
@@ -221,5 +228,7 @@ func (f *win32File) SetWriteDeadline(t time.Time) error { | |||
} | |||
|
|||
func (f *win32File) Flush() error { | |||
return syscall.FlushFileBuffers(f.handle) | |||
err := syscall.FlushFileBuffers(f.handle) | |||
runtime.KeepAlive(f) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
runtime.KeepAlive(f) [](start = 1, length = 20)
should we just remove the finalizer on win32File?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That works for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also removing BackupFileReader finalizer
fileinfo.go
Outdated
@@ -36,6 +38,8 @@ func SetFileBasicInfo(f *os.File, bi *FileBasicInfo) error { | |||
if err := setFileInformationByHandle(syscall.Handle(f.Fd()), fileBasicInfo, (*byte)(unsafe.Pointer(bi)), uint32(unsafe.Sizeof(*bi))); err != nil { | |||
return &os.PathError{Op: "SetFileInformationByHandle", Path: f.Name(), Err: err} | |||
} | |||
runtime.KeepAlive(f) | |||
runtime.KeepAlive(bi) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
runtime.KeepAlive(bi) [](start = 1, length = 21)
I don't think this is necessary; the cgo rules should keep bi alive across the call to setFileInformationByHandle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one I'm unsure about. After (*byte)(unsafe.Pointer(bi))
returns the pointer, is bi
still considered live? How do the new runtime argument liveness rules apply to cgo rules? bi
is no longer accessible as soon as unsafe.Pointer
returns, so based on my understanding, a finalizer could run on bi
before the call to c.
It doesn't matter though, as bi
does not have a finalizer on it, so I can remove this regardless, but I don't see any documents regarding the new liveness rules when passing pointers to cgo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unsafe.Pointer(x) and (*byte)(x) are casts, not function calls, so they're presumably handled differently. So I don't think that's true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, ok. Removed.
pipe.go
Outdated
if err != nil { | ||
return 0, &os.PathError{Op: "open", Path: path, Err: err} | ||
} | ||
runtime.KeepAlive(sa) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
runtime.KeepAlive(sa) [](start = 1, length = 21)
ditto here
pipe.go
Outdated
var sa securityAttributes | ||
sa.Length = uint32(unsafe.Sizeof(sa)) | ||
sa := &securityAttributes{} | ||
sa.Length = uint32(unsafe.Sizeof(*sa)) | ||
if securityDescriptor != nil { | ||
sa.SecurityDescriptor = &securityDescriptor[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
&securityDescriptor[0] [](start = 26, length = 22)
This actually violates cgo rules: https://golang.org/cmd/cgo/
Probably we should be duplicating this into a C-allocated buffer.
I'm worried that b could be collected before asyncIo returns... I think we need to keep it alive explicitly. I'm not sure if KeepAlive directly on a slice does anything or if you need KeepAlive on a pointer to the first element of the slice. (Technically this violates cgo rules, but we don't have an alternative other than copying the data to a C buffer before doing the WriteFile.) Refers to: file.go:216 in bb2dfe9. [](commit_id = bb2dfe9, deletion_comment = False) |
🕐 |
Tested via manual GC invocations, Adding the KeepAlives |
Updated. |
file.go
Outdated
@@ -221,5 +228,6 @@ func (f *win32File) SetWriteDeadline(t time.Time) error { | |||
} | |||
|
|||
func (f *win32File) Flush() error { | |||
return syscall.FlushFileBuffers(f.handle) | |||
err := syscall.FlushFileBuffers(f.handle) | |||
return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unnecessary
Looks good after a minor tweak and separating the CBytes thing out as a separate commit. |
Signed-off-by: Darren Stahl <darst@microsoft.com>
Signed-off-by: Darren Stahl <darst@microsoft.com>
Split into two commits and nit fixed. |
Signed-off-by: Darren Stahl <darst@microsoft.com>
Automatic merge from submit-queue Support windows in dockershim **What this PR does / why we need it**: This is the 2nd part for #45927 . The non-cri implementation dockertools was removed from kubelet v1.7 . Part of previous work for supporting windows container lies in v1.6 dockertools, this PR is to port them to dockershim. Main reference file in v1.6 dockertools windows support: https://github.com/kubernetes/kubernetes/blob/v1.6.4/pkg/kubelet/dockertools/docker_manager_windows.go **Which issue this PR fixes** 45927, for now catching up the implementation of v1.6 **Special notes for your reviewer**: The code change includes 4 parts, put them together as we discussed in #46089 1. Update go-winio package to a newer version 'go-winio' package is used by docker client. This change is to bring the support for Go v1.8, specifically included in the PR: microsoft/go-winio#48 Otherwise it will produce a lot of error like in: fsouza/go-dockerclient#648 2. Add os dependent getSecurityOpts helper method. seccomp not supported on windows Corresponding code in v1.6: https://github.com/kubernetes/kubernetes/blob/v1.6.4/pkg/kubelet/dockertools/docker_manager_windows.go#L78 3. Add updateCreateConfig. Allow user specified network mode setting. This is to be compatible with what kube-proxy package does on Windows. Also, there is a Linux section in both sandbox config and container config: LinuxPodSandboxConfig, LinuxContainerConfig. And that section later goes to Config and HostConfig section under docker container createConfig. Ideally hostconfig section should be dependent on host os, while config should depend on container image os. To simplify the case, here it assumes that windows host supports windows type container image only. It needs to be updated when kubernetes is to support windows host running linux container image or the like. Corresponding code in v1.6: https://github.com/kubernetes/kubernetes/blob/v1.6.4/pkg/kubelet/dockertools/docker_manager_windows.go#L57 4. Add podIpCache in dockershim. For v1.6 windows implementation, it still does not use sandbox, thus only allow single container to be exposed. Here added a cache for saving container IP, to get adapted to the new CRI api. Corresponding code in v1.6: No sandbox: https://github.com/kubernetes/kubernetes/blob/v1.6.4/pkg/kubelet/dockertools/docker_manager_windows.go#L66 Use container id as pod ip: https://github.com/kubernetes/kubernetes/blob/v1.6.4/pkg/kubelet/dockertools/docker_manager.go#L2727 **Release note**:
* Removed debug file * Formatted with gofmt after files were modified via GitHub web interface * Updated go-winio dependency to latest version. This update includes fixes from this PR: microsoft/go-winio#48 That fixes: microsoft/go-winio#41 And should fix #111.
This update includes fixes from this PR: microsoft/go-winio#48 That fixes: microsoft/go-winio#41 And should fix #111.
* Removed debug file * Formatted with gofmt after files were modified via GitHub web interface * Updated go-winio dependency to latest version. This update includes fixes from this PR: microsoft/go-winio#48 That fixes: microsoft/go-winio#41 And should fix #111.
fixes #41
As far as I can tell, this should contain all the runtime.KeepAlive calls needed to conform to the new argument liveness guarantees in Go1.8.x
It turns out that the main issue was due to passing a Go pointer via native calls to the ioCompletionProcessor, causing the new garbage collection rules to cleanup the struct containing the channel even though it did not have a finalizer.
/cc @jhowardmsft @jstarks PTAL
Thanks @mappu for the initial investigation and repro test.
This also updates zsyscall_windows.go with the new 1.8 go generated code, though it is not related to the KeepAlive, it should be done with the update to 1.8.
Signed-off-by: Darren Stahl darst@microsoft.com