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.

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

os/signal: TestCtrlBreak fails on windows/386 #10215

Closed
alexbrainman opened this issue Mar 23, 2015 · 7 comments
Closed

os/signal: TestCtrlBreak fails on windows/386 #10215

alexbrainman opened this issue Mar 23, 2015 · 7 comments
Labels
FrozenDueToAge
Milestone
Go1.5

Comments

@alexbrainman
Copy link
Member

TestCtrlBreak fails on windows/386

--- FAIL: TestCtrlBreak (2.21s)
signal_windows_test.go:101: Program exited with error: exit status -1073741510
FAIL
FAIL os/signal 2.280s

https://storage.googleapis.com/go-build-log/7dd1854f/windows-386-gce_0e29ac61.log

but succeeds on windows/amd64. I cannot reproduce it here. Strange.

The exit status is STATUS_CONTROL_C_EXIT. I can reproduce the failure, if I disable our C-break handler (comment out call to SetConsoleCtrlHandler in runtime).

Alex
@bradfitz bradfitz added this to the Go1.5 milestone Mar 23, 2015
@minux
Copy link
Member

minux commented Mar 23, 2015 via email

@alexbrainman
Copy link
Member Author

I was thinking about the race (in our test). But I don't see how. Parent process waits for 1 second. Surely child should be finished installing the handler. Perhaps I can change the test for parent to read child's output for some "ready" message, but I am grasping for straws here. It would be nice if I can reproduce it here. It also does not break on windows/amd64. Why?

@minux
Copy link
Member

minux commented Mar 24, 2015

could we skip the test for now? I want to do a final trybot run on my external linking
CLs and submit them.

I've sent CL 7971 to skip the test on windows/386.

minux added a commit that referenced this issue Mar 24, 2015
@minux
os/signal: skip TestCtrlBreak on windows/386 temporarily
398f56f 
Update #10215.

Change-Id: Ib588f90279a4ef5461492553d50ad77c742b3560
Signed-off-by: Shenghou Ma <minux@golang.org>
Reviewed-on: https://go-review.googlesource.com/7971
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
@alexbrainman
Copy link
Member Author

I lied. I can reproduce it here. I did git bisect and that is what I find:

b115c35 is the first bad commit
commit b115c35
Author: Russ Cox rsc@golang.org
Date: Wed Mar 18 17:26:36 2015 -0400

cmd/internal/gc: move cgen, regalloc, et al to portable code

This CL moves the bulk of the code that has been copy-and-pasted
since the initial 386 port back into a shared place, cutting 5 copies to 1.

The motivation here is not cleanup per se but instead to reduce the
cost of introducing changes in shared concepts like regalloc or general
expression evaluation. For example, a change after this one will
implement x.(*T) without a call into the runtime. This CL makes that
followup work 5x easier.

The single copy still has more special cases for architecture details
than I'd like, but having them called out explicitly like this at least
opens the door to generalizing the conditions and smoothing out
the distinctions in the future.

This is a LARGE CL. I started by trying to pull in one function at a time
in a sequence of CLs and it became clear that everything was so
interrelated that it had to be moved as a whole. Apologies for the size.

It is not clear how many more releases this code will matter for;
eventually it will be replaced by Keith's SSA work. But as noted above,
the deduplication was necessary to reduce the cost of working on
the current code while we have it.

Passes tests on amd64, 386, arm, and ppc64le.
Can build arm64 binaries but not tested there.
Being able to build binaries means it is probably very close.

Change-Id: I735977f04c0614f80215fb12966dfe9bbd1f5861
Reviewed-on: https://go-review.googlesource.com/7853
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

Alex

@mattn
Copy link
Member

mattn commented Mar 24, 2015

signal handling doesn't work?

package main

import (
    "log"
    "os"
    "os/signal"
    "time"
)

func main() {
    c := make(chan os.Signal, 10)
    signal.Notify(c)
    select {
    case s := <-c:
        log.Println("catch!")
        if s != os.Interrupt {
            log.Fatalf("Wrong signal received: got %q, want %q\n", s, os.Interrupt)
        }
    case <-time.After(3 * time.Second):
        log.Fatalf("Timeout waiting for Ctrl+Break\n")
    }
}
go build
ctrlbreak

And type CTRL-BREAK or CTRL-C, But catch! doesn't appear.

@mattn
Copy link
Member

mattn commented Mar 24, 2015

os1_windows.go

func ctrlhandler1(_type uint32) uint32 {
    var s uint32

    switch _type {
    case _CTRL_C_EVENT, _CTRL_BREAK_EVENT:
        s = _SIGINT
    default:
        return 0
    }

    if sigsend(s) {
        println(1) // ADD PRINTLN HERE
        return 1
    }
    exit(2) // SIGINT, SIGTERM, etc
    return 0
}

When added println in above, it works fine. But not, it doesn't work correctly.

@mattn mattn mentioned this issue Mar 24, 2015
Closed
@minux
Copy link
Member

minux commented Mar 24, 2015 via email

@golang golang locked and limited conversation to collaborators Jun 25, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.5
Development

No branches or pull requests
5 participants
@bradfitz @mattn @minux @gopherbot @alexbrainman