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 #4124 - autobahn tests for jetty, javax websocket APIs #4147

Merged
merged 3 commits into from
Nov 7, 2019

Conversation

lachlan-roberts
Copy link
Contributor

Issue #4124
Added autobahn tests to allow testing of the jetty and javax APIs instead of only core.
Also fixed some problems stopping the tests from passing.

  • No longer selecting duplicate extensions in the javax ContainerDefaultConfigurator.
  • Correctly copy payloads for ping frames in jetty & javax FrameHandlers.
  • Add ByteBuffer to javadoc for @OnWebSocketMessage annotation in jetty API.
  • Cleaned up some test logging for EventSocket.
  • Add autoFragment and maxFrameSize settings to WebSocketPolicy.
  • Fix early validation for multiple extensions in ServletUpgradeResponse.setExtensions.

Signed-off-by: Lachlan Roberts lachlan@webtide.com

- do not select duplicate extensions in javax default Configurator
- correctly copy payload for ping frames in jetty & javax frame handlers
- add ByteBuffer to javadoc for onMessage in jetty api
- cleaned up some test logging for EventSocket
- add autoFragment and maxFrameSize settings to WebSocketPolicy
- fix early validation for multiple extensions in setExtensions

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@joakime joakime self-requested a review October 2, 2019 10:59
@joakime joakime added the Bug For general bugs on Jetty side label Oct 2, 2019
@joakime joakime added this to the 10.0.x milestone Oct 2, 2019
@lachlan-roberts lachlan-roberts requested a review from gregw October 9, 2019 03:00
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.

LGTM. Is this something that we can now run regularly on jenkins?

@lachlan-roberts
Copy link
Contributor Author

@gregw Not in its current state.

The javax autobahn tests are failing because of #4148 / #4152. Once this is fixed it would be possible to run the autobahn tests for all 3 APIs on jenkins with a few changes, but only if we exclude the permessage-deflate server tests (excluding them you can run all the tests in a few seconds otherwise it can take over 20min).

@olamy
Copy link
Member

olamy commented Oct 9, 2019

@lachlan-roberts those autobahn tests need to be executed via command line. That's not related to the current maven plugin we use?

@lachlan-roberts
Copy link
Contributor Author

@olamy Yeah I was just running these ones on the command line for now, I think the current maven plugin is still only using the core API. Do you think it would be possible to eventually set up all three APIs to run test on jenkins for both the server and client tests?

@olamy
Copy link
Member

olamy commented Oct 9, 2019

@lachlan-roberts we need a new version of https://github.com/normanmaurer/autobahntestsuite-maven-plugin this plugin is not very up2date. I can give a try to modify it and add the last autobahn py scripts etc...

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.

Got some things to restore, and a default implementation to correct.
Plus a bad assumption on extensions to fix.

negotiatedExtensions.add(ext);
}

return negotiatedExtensions;
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't right.

A few points ...

  • Multiple copies of the same extension are allowed. Eg: identity; step=1, permessage-deflate, identity; step=2
  • This API should validate each requested name against the list in installed and not return requested that does not show up in installed.

Copy link
Contributor

Choose a reason for hiding this comment

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

The permessage-deflate extension has special language that it reserves the RSV1 bit.
So if you have a offered extensions of identity; step=1, permessage-deflate, identity; step=2, permessage-deflate; client_no_context_takeover, then what happens is ...

  1. identity; step=1 is initialized successfully.
  2. permessage-deflate is initialized, and reserves the RSV1 bit. successfully.
  3. identity; step=2 is initialized successfully.
  4. permessage-deflate; client_no_context_takeover is initialized, and fails to reserve the RSV1 bit, throwing an exception, and erroring out the handshake. (the language used in RFC6455 is MUST immediately _Fail the WebSocket Connection_)

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 don't think this is how it works, I'm not so sure with the identity extension case, but I know that with permessage-deflate the client can offer the same extension multiple times with different parameters and the server can choose the one to use. It would not result in a failed handshake.

RFC6455 Section 9.1

Any interactions
between multiple extensions MAY be defined in the documents defining
the extensions.  In the absence of such definitions, the
interpretation is that the header fields listed by the client in its
request represent a preference of the header fields it wishes to use,
with the first options listed being most preferable.

RFC7692 'Compression Extensions' Section 5

A client may also offer multiple
PMCE choices to the server by including multiple elements in the
"Sec-WebSocket-Extensions" header, one for each PMCE offered.  This
set of elements MAY include multiple PMCEs with the same extension
name to offer the possibility to use the same algorithm with
different configuration parameters.

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants