restore reason string behaviour pre 0.18

[edevil]

Before 0.18.0 the reason string for an RPC error did not interpolate the kind inside. This makes sense because the type already has its own attribute.

This patch restores the original behaviour so that clients that rely on inspecting this error string can continue to operate as before.

[dwrensha]

For reference, here's the commit where the change happened: 13ccb48.
I'm trying to understand if there was a reason for it...

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 52.38%. Comparing base (ab342b3) to head (90d0f47).
Report is 123 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #539      +/-   ##
==========================================
+ Coverage   51.64%   52.38%   +0.73%     
==========================================
  Files          69       70       +1     
  Lines       33735    34578     +843     
==========================================
+ Hits        17422    18113     +691     
- Misses      16313    16465     +152     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[dwrensha]

@ariel-miculas, this pull request is proposing to revert one detail of your commit 13ccb48. Do you care to weigh in? Any reason that I shouldn't accept this change?

[dwrensha]

OK, I reminded myself what's going on here.

Before 0.18.0 the reason string for an RPC error did not interpolate the kind inside. This makes sense because the type already has its own attribute.

capnp-v0.18 added a bunch of variants to the ErrorKind enum (previously only the first four existed):

capnproto-rust/capnp/src/lib.rs

Lines 180 to 395 in 1ec4709

	pub enum ErrorKind {
	/// Something went wrong
	Failed,
	/// The call failed because of a temporary lack of resources. This could be space resources
	/// (out of memory, out of disk space) or time resources (request queue overflow, operation
	/// timed out).
	///
	/// The operation might work if tried again, but it should NOT be repeated immediately as this
	/// may simply exacerbate the problem.
	Overloaded,
	/// The call required communication over a connection that has been lost. The caller will need
	/// to re-establish connections and try again.
	Disconnected,
	/// The requested method is not implemented. The caller may wish to revert to a fallback
	/// approach based on other methods.
	Unimplemented,
	/// Buffer is not large enough
	BufferNotLargeEnough,
	/// Cannot create a canonical message with a capability
	CannotCreateACanonicalMessageWithACapability,
	/// Cannot set AnyPointer field to a primitive value
	CannotSetAnyPointerFieldToAPrimitiveValue,
	/// Don't know how to handle non-STRUCT inline composite.
	CantHandleNonStructInlineComposite,
	/// Empty buffer
	EmptyBuffer,
	/// Empty slice
	EmptySlice,
	/// Enum value or union discriminant {} was not present in schema
	EnumValueOrUnionDiscriminantNotPresent(NotInSchema),
	/// Called get_writable_{data|text}_pointer() but existing list pointer is not byte-sized.
	ExistingListPointerIsNotByteSized,
	/// Existing list value is incompatible with expected type.
	ExistingListValueIsIncompatibleWithExpectedType,
	/// Called get_writable_{data|text|list|struct_list}_pointer() but existing pointer is not a list.
	ExistingPointerIsNotAList,
	/// Expected a list or blob.
	ExpectedAListOrBlob,
	/// Expected a pointer list, but got a list of data-only structs
	ExpectedAPointerListButGotAListOfDataOnlyStructs,
	/// Expected a primitive list, but got a list of pointer-only structs
	ExpectedAPrimitiveListButGotAListOfPointerOnlyStructs,
	/// failed to fill the whole buffer
	FailedToFillTheWholeBuffer,
	/// field and default mismatch
	FieldAndDefaultMismatch,
	/// field not found
	FieldNotFound,
	/// Found bit list where struct list was expected; upgrading boolean lists to struct lists is no longer supported
	FoundBitListWhereStructListWasExpected,
	/// Found struct list where bit list was expected.
	FoundStructListWhereBitListWasExpected,
	/// Cannot represent 4 byte length as `usize`. This may indicate that you are running on 8 or 16 bit platform or message is too large.
	FourByteLengthTooBigForUSize,
	/// Cannot represent 4 byte segment length as usize. This may indicate that you are running on 8 or 16 bit platform or segment is too large
	FourByteSegmentLengthTooBigForUSize,
	/// group field but type is not Struct
	GroupFieldButTypeIsNotStruct,
	/// init() is only valid for struct and AnyPointer fields
	InitIsOnlyValidForStructAndAnyPointerFields,
	/// initn() is only valid for list, text, or data fields
	InitnIsOnlyValidForListTextOrDataFields,
	/// InlineComposite list with non-STRUCT elements not supported.
	InlineCompositeListWithNonStructElementsNotSupported,
	/// InlineComposite list's elements overrun its word count.
	InlineCompositeListsElementsOverrunItsWordCount,
	/// InlineComposite lists of non-STRUCT type are not supported.
	InlineCompositeListsOfNonStructTypeAreNotSupported,
	/// Too many or too few segments {segment_count}
	InvalidNumberOfSegments(usize),
	/// Invalid segment id {id}
	InvalidSegmentId(u32),
	/// List(AnyPointer) not supported.
	ListAnyPointerNotSupported,
	/// List(Capability) not supported
	ListCapabilityNotSupported,
	/// Malformed double-far pointer.
	MalformedDoubleFarPointer,
	/// Message contains invalid capability pointer.
	MessageContainsInvalidCapabilityPointer,
	/// Message contains list pointer of non-bytes where data was expected.
	MessageContainsListPointerOfNonBytesWhereDataWasExpected,
	/// Message contains list pointer of non-bytes where text was expected.
	MessageContainsListPointerOfNonBytesWhereTextWasExpected,
	/// Message contains list with incompatible element type.
	MessageContainsListWithIncompatibleElementType,
	/// Message contains non-capability pointer where capability pointer was expected.
	MessageContainsNonCapabilityPointerWhereCapabilityPointerWasExpected,
	/// Message contains non-struct pointer where struct pointer was expected.
	MessageContainsNonStructPointerWhereStructPointerWasExpected,
	/// Message contains non-list pointer where data was expected.
	MessageContainsNonListPointerWhereDataWasExpected,
	/// Message contains non-list pointer where list pointer was expected
	MessageContainsNonListPointerWhereListPointerWasExpected,
	/// Message contains non-list pointer where text was expected.
	MessageContainsNonListPointerWhereTextWasExpected,
	/// Message contains null capability pointer.
	MessageContainsNullCapabilityPointer,
	/// Message contains out-of-bounds pointer,
	MessageContainsOutOfBoundsPointer,
	/// Message contains text that is not NUL-terminated
	MessageContainsTextThatIsNotNULTerminated,
	/// Message ends prematurely. Header claimed {header} words, but message only has {body} words,
	MessageEndsPrematurely(usize, usize),
	/// Message is too deeply nested.
	MessageIsTooDeeplyNested,
	/// Message is too deeply-nested or contains cycles.
	MessageIsTooDeeplyNestedOrContainsCycles,
	/// Message was not aligned by 8 bytes boundary. Either ensure that message is properly aligned or compile `capnp` crate with \"unaligned\" feature enabled.
	MessageNotAlignedBy8BytesBoundary,
	/// Message's size cannot be represented in usize
	MessageSizeOverflow,
	/// Message is too large
	MessageTooLarge(usize),
	/// Nesting limit exceeded
	NestingLimitExceeded,
	/// Not a struct
	NotAStruct,
	/// Only one of the section pointers is pointing to ourself
	OnlyOneOfTheSectionPointersIsPointingToOurself,
	/// Packed input did not end cleanly on a segment boundary.
	PackedInputDidNotEndCleanlyOnASegmentBoundary,
	/// Premature end of file
	PrematureEndOfFile,
	/// Premature end of packed input.
	PrematureEndOfPackedInput,
	/// Read limit exceeded
	ReadLimitExceeded,
	/// setting dynamic capabilities is unsupported
	SettingDynamicCapabilitiesIsUnsupported,
	/// Struct reader had bitwidth other than 1
	StructReaderHadBitwidthOtherThan1,
	/// Text blob missing NUL terminator.
	TextBlobMissingNULTerminator,
	/// Text contains non-utf8 data
	TextContainsNonUtf8Data(core::str::Utf8Error),
	/// Tried to read from null arena
	TriedToReadFromNullArena,
	/// type mismatch
	TypeMismatch,
	/// Detected unaligned segment. You must either ensure all of your segments are 8-byte aligned,
	/// or you must enable the "unaligned" feature in the capnp crate
	UnalignedSegment,
	/// Unexpected far pointer
	UnexpectedFarPointer,
	/// Unknown pointer type.
	UnknownPointerType,
	}

However, the RPC Exception.Type union still only has four variants:

capnproto-rust/capnp-rpc/schema/rpc.capnp

Lines 1198 to 1246 in 1ec4709

	enum Type {
	failed @0;
	# A generic problem occurred, and it is believed that if the operation were repeated without
	# any change in the state of the world, the problem would occur again.
	#
	# A client might respond to this error by logging it for investigation by the developer and/or
	# displaying it to the user.
	overloaded @1;
	# The request was rejected due to a temporary lack of resources.
	#
	# Examples include:
	# - There's not enough CPU time to keep up with incoming requests, so some are rejected.
	# - The server ran out of RAM or disk space during the request.
	# - The operation timed out (took significantly longer than it should have).
	#
	# A client might respond to this error by scheduling to retry the operation much later. The
	# client should NOT retry again immediately since this would likely exacerbate the problem.
	disconnected @2;
	# The method failed because a connection to some necessary capability was lost.
	#
	# Examples include:
	# - The client introduced the server to a third-party capability, the connection to that third
	# party was subsequently lost, and then the client requested that the server use the dead
	# capability for something.
	# - The client previously requested that the server obtain a capability from some third party.
	# The server returned a capability to an object wrapping the third-party capability. Later,
	# the server's connection to the third party was lost.
	# - The capability has been revoked. Revocation does not necessarily mean that the client is
	# no longer authorized to use the capability; it is often used simply as a way to force the
	# client to repeat the setup process, perhaps to efficiently move them to a new back-end or
	# get them to recognize some other change that has occurred.
	#
	# A client should normally respond to this error by releasing all capabilities it is currently
	# holding related to the one it called and then re-creating them by restoring SturdyRefs and/or
	# repeating the method calls used to create them originally. In other words, disconnect and
	# start over. This should in turn cause the server to obtain a new copy of the capability that
	# it lost, thus making everything work.
	#
	# If the client receives another `disconnected` error in the process of rebuilding the
	# capability and retrying the call, it should treat this as an `overloaded` error: the network
	# is currently unreliable, possibly due to load or other temporary issues.
	unimplemented @3;
	# The server doesn't implement the requested method. If there is some other method that the
	# client could call (perhaps an older and/or slower interface), it should try that instead.
	# Otherwise, this should be treated like `failed`.
	}

The rpc::from_error() function converts a ErrorKind into an Exception.Type. If the ErrorKind is not one of the four primary kinds, then it gets mapped to Failed. We do builder.set_reason(error.to_string()) so that the extra information represented by ErrorKind variant is not lost; instead of just dropping that information on the floor, we allow it to influence what gets put in the reason string field.

Note that in the base capnp crate most errors do not write to the Error::extra field. So the change proposed in this PR would result in information about such errors being lost when crossing RPC boundaries.

Anyways -- I am aware that the mismatch between these error types is not ideal, and I would like to figure out a way to make it nicer.

[dwrensha]

@edevil what do you think about #540?

[edevil]

I think it makes sense. The case I had issues with were exceptions of type Failed, and with the linked change I have complete control over the reason string.

[dwrensha]

Closing in favor of #540.