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

rpc: update win-library + use atomics #27829

Closed
wants to merge 1 commit into from

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Aug 1, 2023

The windows named-pipe implementation hasn't been kept up to date for the last seven years, this PR switches it over to a somewhat more active fork. I want to see if it builds successfully on windows/386, which most often is failing with errors like

panic: The handle is invalid.
goroutine 10385 [running]:
internal/poll.execIO(0xa5eedd4, 0xce9620)
	C:/Users/appveyor/AppData/Local/geth-go-1.20.6-windows-amd64/go/src/internal/poll/fd_windows.go:201 +0x44f
internal/poll.(*FD).Read(0xa5eedc0, {0xb1db000, 0x1000, 0x1000})
	C:/Users/appveyor/AppData/Local/geth-go-1.20.6-windows-amd64/go/src/internal/poll/fd_windows.go:436 +0x13b
net.(*netFD).Read(0xa5eedc0, {0xb1db000, 0x1000, 0x1000})
	C:/Users/appveyor/AppData/Local/geth-go-1.20.6-windows-amd64/go/src/net/fd_posix.go:55 +0x3f
net.(*conn).Read(0xb199728, {0xb1db000, 0x1000, 0x1000})
	C:/Users/appveyor/AppData/Local/geth-go-1.20.6-windows-amd64/go/src/net/net.go:183 +0x4f
net/http.(*persistConn).Read(0xa9e2780, {0xb1db000, 0x1000, 0x1000})
	C:/Users/appveyor/AppData/Local/geth-go-1.20.6-windows-amd64/go/src/net/http/transport.go:1943 +0x141
bufio.(*Reader).fill(0xa687170)
	C:/Users/appveyor/AppData/Local/geth-go-1.20.6-windows-amd64/go/src/bufio/bufio.go:106 +0xe9
bufio.(*Reader).Peek(0xa687170, 0x1)
	C:/Users/appveyor/AppData/Local/geth-go-1.20.6-windows-amd64/go/src/bufio/bufio.go:144 +0x6d
net/http.(*persistConn).readLoop(0xa9e2780)
	C:/Users/appveyor/AppData/Local/geth-go-1.20.6-windows-amd64/go/src/net/http/transport.go:2107 +0x1df
created by net/http.(*Transport).dialConn
	C:/Users/appveyor/AppData/Local/geth-go-1.20.6-windows-amd64/go/src/net/http/transport.go:1765 +0x1480
FAIL	github.com/ethereum/go-ethereum/rpc	14.792s

and

ok  	github.com/ethereum/go-ethereum/rlp/rlpgen	3.318s
runtime.preemptM: duplicatehandle failed; errno=6
fatal error: runtime.preemptM: duplicatehandle failed
runtime stack:
runtime.throw({0x132f707?, 0xffffffffffffffff?})
	C:/Users/appveyor/AppData/Local/geth-go-1.20.6-windows-amd64/go/src/runtime/panic.go:1047 +0x65 fp=0xd5731ff870 sp=0xd5731ff840 pc=0xe6a7e5
runtime.preemptM(0xd5731ffdf0?)
	C:/Users/appveyor/AppData/Local/geth-go-1.20.6-windows-amd64/go/src/runtime/os_windows.go:1345 +0x4a5 fp=0xd5731ffdd0 sp=0xd5731ff870 pc=0xe67f85
runtime.preemptone(0xc006023800?)
	C:/Users/appveyor/AppData/Local/geth-go-1.20.6-windows-amd64/go/src/runtime/proc.go:5572 +0x5e fp=0xd5731ffde8 sp=0xd5731ffdd0 pc=0xe788fe
runtime.preemptall()
	C:/Users/appveyor/AppData/Local/geth-go-1.20.6-windows-amd64/go/src/runtime/proc.go:5534 +0x55 fp=0xd5731ffe20 sp=0xd5731ffde8 pc=0xe78855
runtime.forEachP(0x1358c50)
	C:/Users/appveyor/AppData/Local/geth-go-1.20.6-windows-amd64/go/src/runtime/proc.go:1676 +0xdb fp=0xd5731ffe88 sp=0xd5731ffe20 pc=0xe6fe7b
runtime.gcMarkDone.func1()
	C:/U

@holiman holiman requested a review from fjl as a code owner August 1, 2023 08:58
@holiman
Copy link
Contributor Author

holiman commented Aug 1, 2023

CI failure for Image: Visual Studio 2019; Environment: GETH_ARCH=386, GETH_MINGW=C:\msys64\mingw32 was

ok  	github.com/ethereum/go-ethereum/eth/catalyst	5.986s
--- FAIL: TestSkeletonSyncRetrievals (2.73s)
    skeleton_test.go:908: test 6, mid state: dropped peers mismatch: have 0, want 1
FAIL
FAIL	github.com/ethereum/go-ethereum/eth/downloader	162.714s

But the rpc tests passed. Rerunning it now

@fjl
Copy link
Contributor

fjl commented Aug 1, 2023

I don't think these failures are related to the named pipe library.

@holiman
Copy link
Contributor Author

holiman commented Aug 2, 2023

I'll re-run the appveyor builds, and update the list below with the status for the 386 build

  1. RPC Success (failed on unrelated)
  2. Success (no fails )
  3. Success
  4. Success
  5. Success (failed on TestSkeletonSyncRetrievals)
  6. Success

@holiman
Copy link
Contributor Author

holiman commented Aug 17, 2023

I don't think these failures are related to the named pipe library.

The reason I suspect they are, is that e.g this panic does arise in the windows file descriptor polling.

C:/Users/appveyor/AppData/Local/geth-go-1.20.6-windows-amd64/go/src/internal/poll/fd_windows.go:201

Copy link
Contributor

@fjl fjl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After looking into the new npipe depency code, I think it probably could have an effect on this error after all. But seems weird to add this dependency.

Alternative to explore: https://github.com/microsoft/go-winio

@holiman
Copy link
Contributor Author

holiman commented Aug 22, 2023

closing in favour of #27972

@holiman holiman closed this Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants