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

Issue #4548 - clean up websocket removing duplicated and unused classes #4549

Merged
merged 19 commits into from
Feb 18, 2020

Conversation

lachlan-roberts
Copy link
Contributor

@lachlan-roberts lachlan-roberts commented Feb 6, 2020

Closes #4548

  • Create new module websocket-util which contains implementation classes shared by websocket-jetty and websocket-javax.
  • Many of these classes had to be changed as the javax and jetty versions of them differed slightly.

Other general cleanups, removed unused interfaces and classes, etc..

lachlan-roberts and others added 12 commits February 3, 2020 18:18
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: olivier lamy <oliver.lamy@gmail.com>
@joakime
Copy link
Contributor

joakime commented Feb 7, 2020

Quite the mess you have here. :-)

The InvokerUtils has 2 versions.

The javax version is the most complete and should be the one we settle on for websocket-core (or even jetty-util)
The ReflectUtil could probably be in jetty-util as well.

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

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

The MessageWriter should not use OutputStreamWriter, as the contract for OutputStreamWriter violates the rules of RFC6455 (we MUST validate outgoing UTF-8 and fail the connection if in violation. OutputStreamWriter will use replacement characters, which is not allowed)

@lachlan-roberts
Copy link
Contributor Author

lachlan-roberts commented Feb 12, 2020

@joakime good catch, I see that this is a problem but it would also be good to not duplicate the implementations, and we probably want to use the ByteBufferPool instead of allocating CharBuffers.

I see that this is also going to be a problem with our MessageReader which I have not changed. It implements InputStreamReader and so will have the same problem with the replacement characters.

I might leave out the changes to MessageWriter, and put a separate issue up (#4538). I'm having enough trouble getting this PR working as is.

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts lachlan-roberts marked this pull request as ready for review February 12, 2020 21:11
@lachlan-roberts
Copy link
Contributor Author

I had to change InvokerUtils so that it gets passed in the MethodHandles.Lookup, this makes it have the same access permissions as the calling module and we don't get lots of IllegalAccessExceptions.

@joakime joakime dismissed their stale review February 13, 2020 14:52

MessageWriter reverted, more work needed in new PR.

@lachlan-roberts
Copy link
Contributor Author

Can I get some reviews on this soon. Any other work in jetty-10 websocket would likely cause lots of merge conflicts as there are so many files changed here.

I also have some further changes based off this branch to fix issues #4538 and #4571. So I can put PRs up for those once this has been merged.

Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

There are aspects of these classes I don't like.... but in general the move to util is good!

@lachlan-roberts lachlan-roberts merged commit 5fe202f into jetty-10.0.x Feb 18, 2020
@lachlan-roberts lachlan-roberts deleted the jetty-10.0.x-WebSocketCleanup branch February 18, 2020 10:43
@joakime
Copy link
Contributor

joakime commented Feb 18, 2020

There are aspects of these classes I don't like.... but in general the move to util is good!

Please file issues about what you don't like.

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.

duplicated classes between jetty and javax websocket implementations
4 participants