Skip to content

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

Merged
lachlan-roberts merged 19 commits intojetty-10.0.xfrom
jetty-10.0.x-WebSocketCleanup
Feb 18, 2020
Merged

Issue #4548 - clean up websocket removing duplicated and unused classes#4549
lachlan-roberts merged 19 commits intojetty-10.0.xfrom
jetty-10.0.x-WebSocketCleanup

Conversation

@lachlan-roberts
Copy link
Copy Markdown
Contributor

@lachlan-roberts lachlan-roberts commented Feb 6, 2020 •
edited
Loading

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
@lachlan-roberts
Replace usage of CompletableFutureCallback with FutureCallback
0565001 
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts
removed unused websocket classes
a392b1f 
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts
Issue #4538 - clean up javax MessageWriter and MessageOutputStream
d9f9dde 
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts
general websocket-jetty-common cleanup and remove unused files
7da5dae 
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts
remove unused javax UpgradeResponse interface
c96c62e 
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts
clean up and rename classes for javax UpgradeRequest interface
6e3e591 
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts
create websocket-util module for common classes from jetty and javax ws
d1358f0 
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts
Merge remote-tracking branch jetty-10.0.x into jetty-10.0.x-WebSocket…
505c475 
…Cleanup
@lachlan-roberts
fix module rename in poms to websocket-util
769db70 
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts
WebSocketComponents can be accessed from CoreSession
d5dc83c 
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts
fix some more tests
83a1fe6 
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@olamy
adjust jpms special hack for reflection and modules
a40936d 
Signed-off-by: olivier lamy <oliver.lamy@gmail.com>
@joakime
Copy link
Copy Markdown
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.

@lachlan-roberts lachlan-roberts mentioned this pull request Feb 9, 2020
Closed
@lachlan-roberts
InvokerUtils now must be given a MethodHandles Lookup instance
6a0c970 
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts
Merge remote-tracking branch 'origin/jetty-10.0.x' into jetty-10.0.x-…
abe4cd9 
…WebSocketCleanup
@lachlan-roberts
Use MethodHandles.privateLookupIn() for endpointClass lookups.
6ff2c5e 
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts
Move MessageSink interface.
038d58d 
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
joakime
joakime previously requested changes Feb 11, 2020
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

lachlan-roberts commented Feb 12, 2020 •
edited
Loading

@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.

@lachlan-roberts
revert MessageWriter implementation
a33129b 
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts
add websocket-util.jar to jetty & javax websocket.mod files
5361491 
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts
attempt to fix OSGI problems
bd97a01 
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
Copy Markdown
Contributor Author

lachlan-roberts commented Feb 12, 2020

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
Copy Markdown
Contributor Author

lachlan-roberts commented Feb 17, 2020

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.

gregw
gregw approved these changes Feb 18, 2020
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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

Reviewers

@olamy olamy olamy approved these changes

@gregw gregw gregw approved these changes

@joakime joakime Awaiting requested review from joakime

@sbordet sbordet Awaiting requested review from sbordet

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

duplicated classes between jetty and javax websocket implementations

4 participants

@lachlan-roberts @joakime @olamy @gregw