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

Remove ORIGIN header from ws opening handshake v13, clean up code #12293

Merged

Conversation

amizurov
Copy link
Sponsor Contributor

@amizurov amizurov commented Apr 7, 2022

Motivation:

In Netty 5 we only left the latest 13 version of websocket, so we can clean up the code a bit.

Modification:

Result:
Removed not used code and fixed wrong behaviour with origin header #9673.

@amizurov
Copy link
Sponsor Contributor Author

@chrisvest @normanmaurer could you please take a look.

@chrisvest
Copy link
Contributor

The Graal build is failing with

2022-04-07T16:15:48.7340933Z Error: Classes that should be initialized at run time got initialized during image building:
2022-04-07T16:15:48.7343421Z  io.netty.util.internal.PlatformDependent was unintentionally initialized at build time. To see why io.netty.util.internal.PlatformDependent got initialized use --trace-class-initialization=io.netty.util.internal.PlatformDependent
2022-04-07T16:15:48.7344128Z 
2022-04-07T16:15:48.7344721Z Error: Use -H:+ReportExceptionStackTraces to print stacktrace of underlying exception
2022-04-07T16:15:48.7695521Z Error: Image build request failed with exit status 1

I don't see anything in the changes that would pull in the 4.1 version of PlatformDependent, but we did have a problem like that a while back, so maybe a rebase will fix it.

Copy link
Contributor

@chrisvest chrisvest left a comment

Choose a reason for hiding this comment

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

Looks fine for me, but I also want Normans review before merging.

@@ -267,12 +232,12 @@ protected FullHttpRequest newHandshakeRequest(BufferAllocator allocator) {
*/
@Override
protected void verify(FullHttpResponse response) {
HttpResponseStatus status = response.status();
var status = response.status();
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what the consensus is on using the var keyword.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

I don't think that for using var exists strict guidelines or something like this.
When i prefer use :

  • type explicitly mentioned in initializer (var request = new DefaultFullHttpRequest(..), var bytes = new byte[10])
  • return type is well known (var status = httpResponse.status(), var headers = httpResponse.headers(), ..)
  • qualified static method (var statusCodes = List.of("1xx", "2xx", "3xx", "4xx", "5xx"))
  • type casting ( if (msg instance of HttpObject) var httpObject = (HttpObject) msg)
  • it can make the line shorter because we have a limit of 120 characters.
    But if you think that this is inappropriate here, I can remove var in hole PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

@doom369 thanks, good guide and It's looks similar with my list, also i never use var for base types (int, byte, .....). By the way, again if you think not use var at all in our code i can remove, no problem here. Or we can approve some cases in its use and follow them otherwise as I mentioned there are no strict rules and each contributor will do it in his own way. @doom369, @hyperxpro, @chrisvest, @normanmaurer WDYT ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be fine with following the guidelines of that cheat sheet.

Copy link
Contributor

@doom369 doom369 Apr 19, 2022

Choose a reason for hiding this comment

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

also i never use var for base types (int, byte, .....).

Same here, also I never do that for the method returns, like in this particular example, as it always complicates the code reading, even the variable name seems super straightforward, type is not clear.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

Fixed, according guidelines.

Copy link
Contributor

@hyperxpro hyperxpro left a comment

Choose a reason for hiding this comment

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

As @chrisvest mentioned, let's refrain from using var because it doesn't give the exact object type.

*/
static byte[] randomBytes(int size) {
byte[] bytes = new byte[size];
var bytes = new byte[size];
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

Left as is

private WebSocketUtil() {
// Unused
throw new UnsupportedOperationException("This is a utility class and cannot be instantiated");
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert because no one is going to initialize this class except us.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

If you think it's redundant, I can remove it. But for me it's more clear in utility classes.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

Removed

…d accept header value only when receive response, cleanup code and add tests.
@amizurov amizurov force-pushed the feature/clean_up_ws_client_handshaker13 branch from 5a411e3 to 84a9e91 Compare April 19, 2022 11:18
@amizurov amizurov requested a review from chrisvest April 19, 2022 11:20
@chrisvest chrisvest merged commit 68f95ec into netty:main Apr 21, 2022
@chrisvest chrisvest added this to the 5.0.0.Alpha2 milestone May 16, 2022
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

5 participants