Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Replace AssertionErrors with RuntimeExceptions #239

Closed
acogoluegnes opened this issue Jan 16, 2017 · 4 comments
Closed

Replace AssertionErrors with RuntimeExceptions #239

acogoluegnes opened this issue Jan 16, 2017 · 4 comments

Comments

@acogoluegnes
Copy link
Contributor

acogoluegnes commented Jan 16, 2017

Use e.g. IllegalStateException instead of AssertionError in BlockingCell and other part of the code (when appropriate). The Error family is too dramatic for this class and client code that tries to handle exceptions from BlockingCell could end up dealing with cases it cannot recover from anyway.

Related to #237.

Reverse also the changes from #237, so the NIO loop doesn't actually handle AssertionError coming from elsewhere than BlockingCell.

@acogoluegnes
Copy link
Contributor Author

See suggestions from @dimas:
#237 (comment)
#237 (comment)

@acogoluegnes acogoluegnes changed the title Use RuntimeExceptions in BlockingCell Replace AssertionErrors with RuntimeExceptions Jan 17, 2017
acogoluegnes added a commit that referenced this issue Feb 23, 2017
@vikinghawk
Copy link
Contributor

vikinghawk commented May 26, 2017

@acogoluegnes

Looks like there are still some instances of Error's in the code base such as UnexpectedFrameError, UnexpectedMethodError, and some places that throw a generic Error. Any chance those places could be switched to RuntimeExceptions too? For example there are a couple places that catch UnsupportedEncodingExceptions and wrap them in an Error. The code could be updated to use Charset.forName() which throws a runtime UnsupportedCharsetException.

@michaelklishin
Copy link
Member

@vikinghawk we would definitely consider that for 5.0. Feel free to submit a PR against master.

@acogoluegnes acogoluegnes reopened this May 29, 2017
@acogoluegnes
Copy link
Contributor Author

Fixed by 3c010f7. Thanks @vikinghawk !

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

No branches or pull requests

3 participants