Skip to content

Conversation

@tegefaulkes
Copy link
Contributor

@tegefaulkes tegefaulkes commented Jan 31, 2024

Description

This removes the use of hard-coded error codes for shutting down quiche streams by using codes provided by reasonToCode

Issues Fixed

Tasks

  • 1. remove hard-coded codes used with streamShutdown and use codes provided by reasonToCode.

Final checklist

  • Domain specific tests
  • Full tests
  • Updated inline-comment documentation
  • Lint fixed
  • Squash and rebased
  • Sanity check the final build

@tegefaulkes tegefaulkes self-assigned this Jan 31, 2024
@ghost
Copy link

ghost commented Jan 31, 2024

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@tegefaulkes tegefaulkes merged commit 1f6fc4b into staging Feb 6, 2024
}
})();
await expect(asd).rejects.toThrow('read 1');
await expect(code).rejects.toThrow('read 0');
Copy link
Member

Choose a reason for hiding this comment

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

I think this test should be more precise, if you are providing a reasonToCode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can add a new issue to expand the tests for this.

Comment on lines +1009 to +1013
this.conn.streamShutdown(
streamId,
Shutdown.Write,
this.reasonToCode('write', errors.ErrorQUICConnectionStopping),
);
Copy link
Member

Choose a reason for hiding this comment

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

Does the end user ever see the "code" for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe so, yeah. Streams get codes when shutdown. Connections get codes and a message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Destroying connections should use reasonToCode for code when rejecting new streams

3 participants