Skip to content

Globally ignore SIGPIPE on Linux #382

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

Merged
merged 1 commit into from
Mar 23, 2021
Merged

Globally ignore SIGPIPE on Linux #382

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Sources/LanguageServerProtocolJSONRPC/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
add_library(LanguageServerProtocolJSONRPC
DisableSigpipe.swift
JSONRPCConnection.swift
MessageCoding.swift
MessageSplitting.swift)
Expand Down
33 changes: 33 additions & 0 deletions Sources/LanguageServerProtocolJSONRPC/DisableSigpipe.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
//===----------------------------------------------------------------------===//
//
// This source file is part of the Swift.org open source project
//
// Copyright (c) 2014 - 2021 Apple Inc. and the Swift project authors
// Licensed under Apache License v2.0 with Runtime Library Exception
//
// See https://swift.org/LICENSE.txt for license information
// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
//
//===----------------------------------------------------------------------===//


#if canImport(Glibc)
import Glibc
#endif

#if os(Linux) || os(Android)
// This is a lazily initialised global variable that when read for the first time, will ignore SIGPIPE.
private let globallyIgnoredSIGPIPE: Bool = {
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels overcomplicated. Just put this as a static variable inside JSONRPCConnection and access it from the initializer. We don't need a public API for this.

Copy link
Member Author

Choose a reason for hiding this comment

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

My line of thinking was that since this is a global property of the process, it should also be toggled by a global function. And I don’t think this really belongs in JSONRPCConnection either. JSONRCPConnection just requests SIGPIPE to be ignored, it shouldn’t need to worry single dispatch etc.

(also: I just copy-pased this code as-is from swift-nio)

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make it an internal function at least.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds like a good compromise 👍

/* no F_SETNOSIGPIPE on Linux :( */
_ = Glibc.signal(SIGPIPE, SIG_IGN)
return true
}()

internal func globallyDisableSigpipe() {
let haveWeIgnoredSIGPIEThisIsHereToTriggerIgnoringIt = globallyIgnoredSIGPIPE
guard haveWeIgnoredSIGPIEThisIsHereToTriggerIgnoringIt else {
fatalError("globallyIgnoredSIGPIPE should always be true")
}
}

#endif
6 changes: 6 additions & 0 deletions Sources/LanguageServerProtocolJSONRPC/JSONRPCConnection.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,12 @@ public final class JSONRPCConnection {
outFD: FileHandle,
syncRequests: Bool = false)
{
#if os(Linux) || os(Android)
// We receive a `SIGPIPE` if we write to a pipe that points to a crashed process. This in particular happens if the target of a `JSONRPCConnection` has crashed and we try to send it a message.
// On Darwin, `DispatchIO` ignores `SIGPIPE` for the pipes handled by it, but that features is not available on Linux.
// Instead, globally ignore `SIGPIPE` on Linux to prevent us from crashing if the `JSONRPCConnection`'s target crashes.
globallyDisableSigpipe()
#endif
state = .created
self.messageRegistry = messageRegistry
self.syncRequests = syncRequests
Expand Down
3 changes: 0 additions & 3 deletions Tests/SourceKitDTests/CrashRecoveryTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,6 @@ final class CrashRecoveryTests: XCTestCase {
}

func testClangdCrashRecovery() throws {
try XCTSkipUnless(isDarwinHost, "rdar://75580936 failing on Linux in CI sometimes")
try XCTSkipUnless(longTestsEnabled)

let ws = try! staticSourceKitTibsWorkspace(name: "ClangCrashRecovery")!
Expand Down Expand Up @@ -190,7 +189,6 @@ final class CrashRecoveryTests: XCTestCase {
}

func testClangdCrashRecoveryReopensWithCorrectBuildSettings() throws {
try XCTSkipUnless(isDarwinHost, "rdar://75580936 failing on Linux in CI sometimes")
try XCTSkipUnless(longTestsEnabled)

let ws = try! staticSourceKitTibsWorkspace(name: "ClangCrashRecoveryBuildSettings")!
Expand Down Expand Up @@ -224,7 +222,6 @@ final class CrashRecoveryTests: XCTestCase {
}

func testPreventClangdCrashLoop() throws {
try XCTSkipUnless(isDarwinHost, "rdar://75580936 failing on Linux in CI sometimes")
try XCTSkipUnless(longTestsEnabled)

let ws = try! staticSourceKitTibsWorkspace(name: "ClangCrashRecovery")!
Expand Down