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

Fix crashes due to mismatching responses sent to the channel when event observer factories fail. #395

Merged
merged 6 commits into from
Mar 8, 2019

Conversation

MrMage
Copy link
Collaborator

@MrMage MrMage commented Mar 6, 2019

No description provided.

@MrMage MrMage requested a review from rebello95 March 6, 2019 16:13
@MrMage
Copy link
Collaborator Author

MrMage commented Mar 6, 2019

A review from @glbrntt would be appreciated as well :-)

@MrMage MrMage closed this Mar 6, 2019
@MrMage MrMage reopened this Mar 6, 2019
@rebello95
Copy link
Collaborator

Happy to take a look after @glbrntt since he probably has a bit more context on this.

Tests/LinuxMain.swift Outdated Show resolved Hide resolved
Tests/SwiftGRPCNIOTests/ServerThrowingTests.swift Outdated Show resolved Hide resolved
Tests/SwiftGRPCNIOTests/ServerThrowingTests.swift Outdated Show resolved Hide resolved
public override func handlerAdded(ctx: ChannelHandlerContext) {
guard let eventObserver = eventObserver,
let context = context else { return }
// Terminate the call if providing an observer fails.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I follow this line, do you mean "Terminate the call if the provided observer fails"?

The below explanation is helpful though! 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added "the future [providing an observer]", hope that makes it more clear: Users return a future containing the observer block. This lets them simply fail that future (with a GRPCStatus) if they do not want to receive messages on this call at all, e.g. in case of authentication failure (e.g. if the request metadata doesn't contain user credentials).

@MrMage
Copy link
Collaborator Author

MrMage commented Mar 6, 2019

Updated the PR :-)

Copy link
Collaborator

@rebello95 rebello95 left a comment

Choose a reason for hiding this comment

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

Some suggestions, but feel free to merge

@testable import SwiftGRPCNIO
import XCTest

private let expectedError = GRPCStatus(code: .internalError, message: "expected error")
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Maybe prefix as kExpectedError since this is a global constant?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Given that this is a private fixture used in just one file, I prefer to not use the k prefix in this case.

Tests/SwiftGRPCNIOTests/ServerThrowingTests.swift Outdated Show resolved Hide resolved
Tests/SwiftGRPCNIOTests/ServerThrowingTests.swift Outdated Show resolved Hide resolved
@MrMage MrMage merged commit 772b78e into master Mar 8, 2019
@MrMage MrMage deleted the event-observer-failure branch April 4, 2019 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants