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

Move message-specific behavior out of generic RPC library #2707

Merged
merged 5 commits into from
May 22, 2019
Merged

Move message-specific behavior out of generic RPC library #2707

merged 5 commits into from
May 22, 2019

Conversation

scrye
Copy link
Contributor

@scrye scrye commented May 22, 2019

This change is Reviewable

@scrye scrye requested a review from oncilla May 22, 2019 12:28
Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

Reviewed 8 of 8 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @scrye)


go/lib/infra/messenger/quic_handler.go, line 98 at r1 (raw file):

}

func messageToSignedPayload(msg *capnp.Message) (*ctrl.SignedPld, error) {

unpacking should be protected with the following against panics.

defer func() {
	if rec := recover(); rec != nil {
		err = common.NewBasicError("pogs panic", nil, "panic", rec)
	}
}()

Should we maybe make cereal.go:parseStruct public? Since this is
essentially code duplication.

Also, there is a bug in cereal.go:parseStruct
The error should not be scoped to the if-statement, please fix while your at it.


go/lib/infra/messenger/quic_handler.go, line 110 at r1 (raw file):

}

func signedPldToMessage(signedPld *ctrl.SignedPld) (*capnp.Message, error) {

The spelling is not consistent.
Pld vs Payload
Proposal

messageToSignedPayload
signedPayloadToMessage

or

// favorite
msgToSignedPld
signedPldToMsg

or

signedPldFromMsg
signedPldToMsg

or

msgToSignedPld
msgFromSignedPld

go/lib/infra/rpc/rpc.go, line 164 at r1 (raw file):

		return nil, err
	}
	msg, err := capnp.NewDecoder(stream).Decode()

Should this be wrapped by a recover?

Copy link
Contributor Author

@scrye scrye left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @oncilla)


go/lib/infra/messenger/quic_handler.go, line 98 at r1 (raw file):

Previously, Oncilla wrote…

unpacking should be protected with the following against panics.

defer func() {
	if rec := recover(); rec != nil {
		err = common.NewBasicError("pogs panic", nil, "panic", rec)
	}
}()

Should we maybe make cereal.go:parseStruct public? Since this is
essentially code duplication.

Also, there is a bug in cereal.go:parseStruct
The error should not be scoped to the if-statement, please fix while your at it.

Done.


go/lib/infra/messenger/quic_handler.go, line 110 at r1 (raw file):

Previously, Oncilla wrote…

The spelling is not consistent.
Pld vs Payload
Proposal

messageToSignedPayload
signedPayloadToMessage

or

// favorite
msgToSignedPld
signedPldToMsg

or

signedPldFromMsg
signedPldToMsg

or

msgToSignedPld
msgFromSignedPld

Done.


go/lib/infra/rpc/rpc.go, line 164 at r1 (raw file):

Previously, Oncilla wrote…

Should this be wrapped by a recover?

Done.

Unit testing this needs interfaces and mocks though, so I skipped it.

Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 7 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r4.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@scrye scrye merged commit dbb6c1c into scionproto:master May 22, 2019
@scrye scrye deleted the pubpr-extract-adapters branch May 22, 2019 14:37
oncilla added a commit to oncilla/scion that referenced this pull request Aug 28, 2019
The recover defer was removed by a code refactor in scionproto#2707.
oncilla added a commit to oncilla/scion that referenced this pull request Aug 29, 2019
The recover defer was removed by a code refactor in scionproto#2707.
oncilla added a commit to oncilla/scion that referenced this pull request Aug 29, 2019
The recover defer was removed by a code refactor in scionproto#2707.
oncilla added a commit that referenced this pull request Aug 29, 2019
The recover defer was removed by a code refactor in #2707.
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.

2 participants