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

Change application reserved error code range #184

Merged
merged 3 commits into from Mar 1, 2017

Conversation

lexs
Copy link
Contributor

@lexs lexs commented Feb 22, 2017

We now have codes up to 204, so application needs to start at 205.

We now have codes up to 204, so application needs to start at 205.
@lexs
Copy link
Contributor Author

lexs commented Feb 22, 2017

Should we bump this number to 300 to allow us to add more error codes?

Protocol.md Outdated
@@ -349,7 +349,7 @@ The Error Data is typically an Exception message, but could include stringified
| __RESERVED__ | 0xFFFFFFFF | __Reserved for Extension Use__ |

__NOTE__: Values in the range of 0x0001 to 0x00FF are reserved for use as SETUP error codes. Values in the range of
0x00101 to 0x001FF are reserved for connection error codes. Values in the range of 0x00201 to 0xFFFFFFFE are reserved for application layer
0x00101 to 0x001FF are reserved for connection error codes. Values in the range of 0x00205 to 0xFFFFFFFE are reserved for application layer
Copy link
Member

@yschimke yschimke Feb 22, 2017

Choose a reason for hiding this comment

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

I think the intent was that errors were in distinct blocks, somewhat like 2XX, 3XX, 4XX, 5XX in HTTP.

So your suggestion to go to 0x300 seems like the only sensible approach. Although should probably be 0x301 as we skip X00 for all other ranges.

Copy link
Contributor

Choose a reason for hiding this comment

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

0x301 seems good to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. 0x301.

@benjchristensen
Copy link
Contributor

@tmontgomery what do you recommend?

@tmontgomery
Copy link
Contributor

Saving some space is good. 0x301 should be OK.

@lexs
Copy link
Contributor Author

lexs commented Feb 23, 2017

Updated to 0x301.

Protocol.md Outdated
@@ -349,7 +349,7 @@ The Error Data is typically an Exception message, but could include stringified
| __RESERVED__ | 0xFFFFFFFF | __Reserved for Extension Use__ |

__NOTE__: Values in the range of 0x0001 to 0x00FF are reserved for use as SETUP error codes. Values in the range of
0x00101 to 0x001FF are reserved for connection error codes. Values in the range of 0x00201 to 0xFFFFFFFE are reserved for application layer
0x00101 to 0x001FF are reserved for connection error codes. Values in the range of 0x00301 to 0xFFFFFFFE are reserved for application layer
Copy link
Member

@yschimke yschimke Feb 23, 2017

Choose a reason for hiding this comment

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

This text currently leaves a gap since it doesn't describe 0x00201 to 0x002FF. Are these predefined application layer errors? App error, rejected, canceled.

0x00301 to 0xFFFFFFFE are custom application layer exceptions?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch. 0x201 to 0x301 should be marked for later use in future versions.

Copy link
Member

Choose a reason for hiding this comment

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

You've confused me more! But it's late here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry. Meant that those (if I recall correctly) are for space for use by later versions of the protocol. "Reserved for future use".

@benjchristensen
Copy link
Contributor

Is this ready for merging?

@yschimke
Copy link
Member

yschimke commented Mar 1, 2017

No, I think it should be changed to explain the 4 ranges
0x0001 to 0x00FF - setup errors
0x0101 to 0x01FF - connection errors
0x0201 to 0x02FF - predefined application errors
0x0301 to 0xFFFFFFFE? - custom application errors

starting custom application errors at 0x0205 seems like it needlessly precludes the spec from ever defining more.

@lexs
Copy link
Contributor Author

lexs commented Mar 1, 2017

Updated the NOTE wording, should be ready now.

@benjchristensen benjchristensen merged commit cfd66ab into rsocket:master Mar 1, 2017
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.

None yet

4 participants