websocket/ee/jakarta/websocket/server/serverendpointconfig/WSClient.java#getUserPropertiesTest TCK challenge

[jansupol]

Websocket 2.1 TCK challenge (Jakarta EE 10) for test websocket/ee/jakarta/websocket/server/serverendpointconfig/WSClient.java#getUserPropertiesTest

Description:

1. In WebSocket 2.0, there is a ServerEndpointConfig class which has getUserProperties method.
2. There also is ServerEndpointConfig#Configurator class, which has modifyHandshake method.
3. ServerEndpointConfig is an interface, whose implementation instance is registered in ServerApplicationConfig#getEndpointConfigs. ServerEndpointConfig.Configurator is returned by ServerEndpointConfig.getConfigurator. Both ServerEndpointConfig and ServerEndpointConfig.Configurator are instances provided by the WebSocket user then.

In WebSocket 2.0, the ServerEndpointConfig#Configurator class, which has modifyHandshake method is called and the user provided instance of ServerEndpointConfig is passed in as an argument.

In WebSocket 2.1, the modifyHandshake method javadoc was altered by:

The user properties made available via ServerEndpointConfig#getUserProperties() must be a per
* WebSocket connection (i.e. per jakarta.websocket.Session) copy of the user properties. This copy,
* including any modifications made to the user properties during the execution of this method must be used to
* populate the initial contents of jakarta.websocket.Session#getUserProperties().

1. The JavaDoc of and interface method usually means the implementation of the method should do what the contract, defined by the JavaDoc, specify. The implementation of the method is however a user code, and the user may not follow the contract. That's exactly what the TCK test does: it provides the implementation of userProperties method that returns a mutable static map.
2. The TCK test expects the WebSocket implementation to ensure the call of the userProperties method from a user implementation of modifyHandshake method will somehow return a copy of the user properties.
3. It is not clear how to achieve this: class instrumentation is not possible (it is an instance that is registered in ServerApplicationConfig#getEndpointConfigs. The only possibility seems to wrap the user instance passed in modifyHandshake. But that is backward-incompatible change - the user used to get his instance, which he could have casted and could have called some user-defined method.
4. The javadoc of modifyHandshake does not inform the user he gets a wrapper instead of his implementation. The backward incompatible change must not be done in a minor release, it must have been done in major release of WebSeocket 3.0.

[jansupol]

See also discussion in #783 starting here and forward.

[jansupol]

The TCK test has been added in January.

[markt-asf]

The change was made to add clarify to an area of the specification that was undefined and the approach taken was chosen by the WebSocket project as it best matched the expectations of users.

I am aware of at least two implementations that have implemented the behaviour described in the updated WebSocket 2.1 Javadoc for at least 8 years without reports of issues from users. At least one of those implementations uses wrapping and again there have been no reports from users of issues trying to cast to the original implementation class in that time.

That said, I accept the backwards compatibility concern. While wrapping has been used successfully for an extended period of time, I believe there is at least one alternative approach that would provide the same behaviour while still exposing the original implementation class. I need to validate that such an approach is viable. I should be able to do that in the next day or two.

[scottmarlow]

Some guidance from the TCK Process 1.2 under "Valid Challenges":

The following scenarios are considered in scope for test challenges:

1. Claims that a test assertion conflicts with the specification.
2. Claims that a test asserts requirements over and above that of the specification.
3. Claims that an assertion of the specification is not sufficiently implementable.
4. Claims that a test is not portable or depends on a particular implementation.

I think both of you already know ^ but I think the point of #4 helps reinforce what Mark just said about the test clarifying an area of spec that was undefined, which I think maps to #4 because it means the test is not portable to all implementations that follow the spec exactly.

[markt-asf]

#4 doesn't apply because both the specification and the TCK were updated. The TCK test is testing exactly what the spec says should happen.

You could argue that none of those 4 points apply but I am happy leaving this challenge open while we figure out the best way to resolve it.

I'm still investigating alternative approaches.

My current thinking is that we'll end up selecting one of the following:

1. A viable alternative approach addresses the backwards compatibility concerns.
2. The specification and TCK changes related to WebSocket issue 235 are reverted
3. We update the WebSocket version in Jakarta EE 10 to 3.0

My current thinking is that option 1 is going to involve some sort of dynamic proxy. The result is always that ServerEndpointConfig#getUserProperties() when called from the modifyHandshake will return the per connection copy of the user properties rather than the original map.

Mainly a question for @jansupol : assuming that all other methods present on the original ServerEndpointConfig instance were still available (hence casting to the original class can be used to access those methods) would that be sufficient to address your backwards compatibility concerns?

[markt-asf]

I've implemented an alternative solution for Tomcat using Javassist that demonstrates that the backwards compatibility issues can be avoided.

[jansupol]

Decisions, decisions...
The Javassist solution looks a bit better than a wrapper, but it comes with some additional requirements:

1. The ServerEndpointConfig must not be non-static inner class
2. It needs to have a public no-arg constructor
3. It needs to have opens <user package> to javassist in the user module-info

[jansupol]

Either of the two can bite the user. I understand the reason behind this, and the reason is good, the solution is difficult:

1. Upgrading WebSocket to 3.0 seems to be a good solution
2. Excluding the test is also an option, but it leaves a grey area.

Another idea is to update the modifyHandshake(ServerEndpointConfig sec, HandshakeRequest request, HandshakeResponse response). HandshakeRequest is not implemented by the user, and it is per-connection, the copy of the properties would better suit there...

[markt-asf]

I'd prefer to avoid re-instating the grey area.
The requirements of the Javaassist solution are not ideal.
If we are thinking about WebSocket 3.0 as the solution then how about something like this:

• New class HandshakeServerEndpointConfig extends ServerEndpointConfig
• Wraps an existing ServerEndpointConfig
• Adds a new method to obtain the wrapped ServerEndpointConfig
• Overrides getUserProperties() to provide a per instance Map that is initially populated with a shallow copy of getUserProeprties() from the wrapped ServerEndpointConfig
• Add a new modifyHandshake() method that uses HandshakeServerEndpointConfig rather than ServerEndpointConfig
• Have the default implementation of that new method call the old modifyHandshake() method
• Deprecate the old modifyHandshake() method

The spec would be updated to state that the container wraps the original ServerEndpointConfig in an instance of HandshakeServerEndpointConfig before calling the new version of modifyHandshake().

The intention is that it is a NO-OP for existing code unless it needs to get hold of the original user properties map.

[jansupol]

That sounds like a great idea! I think that solves all the issues and concerns.

[markt-asf]

@joakime WDYT?

[joakime]

Websocket 2.1 TCK challenge (Jakarta EE 10) for test websocket/ee/jakarta/websocket/server/serverendpointconfig/WSClient.java#getUserPropertiesTest

Description:

1. In WebSocket 2.0, there is a ServerEndpointConfig class which has getUserProperties method.
2. There also is ServerEndpointConfig#Configurator class, which has modifyHandshake method.
3. ServerEndpointConfig is an interface, whose implementation instance is registered in ServerApplicationConfig#getEndpointConfigs. ServerEndpointConfig.Configurator is returned by ServerEndpointConfig.getConfigurator. Both ServerEndpointConfig and ServerEndpointConfig.Configurator are instances provided by the WebSocket user then.

Don't follow your point here.
This is the application provided of ServerEndpointConfig for registering new endpoints.
An implementation of Jakarta WebSocket Server is free to wrap those ServerEndpointConfig to suit it's own needs (extra details, context, caching, etc).
This includes container initialization of either discovered @ServerEndpoint(config=<class>) annotations, zero or more ServerApplicationConfig.getEndpointConfigs() (discovered before container init), or manual usage of the ServerContainer.addEndpoint(EndpointConfig).

In WebSocket 2.0, the ServerEndpointConfig#Configurator class, which has modifyHandshake method is called and the user provided instance of ServerEndpointConfig is passed in as an argument.

The user provided ServerEndpointConfig can be wrapped or supplemented by the container. (in practice, nearly all current implementations of the jakarta websocket spec performs wrapping on the ServerEndpointConfig now)
The raw application provided ServerEndpointConfig is not passed in, nor is an application expected to be able to cast to your implementation.
(What if the server container needs to supplement the .getUserProperties() from server level config, or a filter, or based on negotiated extensions, or use a dynamic class for .getEndpointClass())

In the old jakarta websocket spec 1.0, if you used the @ServerEndpoint(config=<class>), then you always had a new ServerEndpointConfig.Configurator per endpoint instance of and a server container provided EndpointConfig instance. This most common form of websocket declaration drove the implementation everywhere else.

In WebSocket 2.1, the modifyHandshake method javadoc was altered by:

The user properties made available via ServerEndpointConfig#getUserProperties() must be a per

• WebSocket connection (i.e. per jakarta.websocket.Session) copy of the user properties. This copy,
• including any modifications made to the user properties during the execution of this method must be used to
• populate the initial contents of jakarta.websocket.Session#getUserProperties().

This is the change discussed at jakartaee/websocket#235 (comment)
This clarification of the behavior takes the spec from being undefined to being defined.
This is not a backward compatible change issue.

1. The JavaDoc of and interface method usually means the implementation of the method should do what the contract, defined by the JavaDoc, specify. The implementation of the method is however a user code, and the user may not follow the contract. That's exactly what the TCK test does: it provides the implementation of userProperties method that returns a mutable static map.

Again, this is just what your application ServerEndpointConfig.Configurator is specifying as it's preferred properties, that Map is not used directly, nor is the ServerEndpointConfig passed as-is into the ServerEndpointConfig.Configurator.

This is how it's been since WebSocket 1.0

2. The TCK test expects the WebSocket implementation to ensure the call of the userProperties method from a user implementation of modifyHandshake method will somehow return a copy of the user properties.

This is still true, as long as the TCK doesn't assert that the UserProperties is the SAME one as returned by EndpointConfig.getUserProperties().
It should also only assert that the UserProperties contains the expected values from the EndpointConfig.getUserProperties(), and not any other properties that might show up from the server level / client level EndpointConfig.

3. It is not clear how to achieve this: class instrumentation is not possible (it is an instance that is registered in ServerApplicationConfig#getEndpointConfigs. The only possibility seems to wrap the user instance passed in modifyHandshake. But that is backward-incompatible change - the user used to get his instance, which he could have casted and could have called some user-defined method.

The wrapping is exactly how most implementations have done this currently.
There has never been the ability to cast to your own ServerEndpointConfig, that's not part of the websocket spec.
Custom methods on your ServerEndpointConfig are not possible to access outside of the scope of the called methods on ServerEndpointConfig.Configurator.
This means you cannot use those custom methods in your onOpen method.

This kind of casting is invalid per websocket spec ...

@OnOpen
public void open(EndpointConfig config, Session session)
{
    MyCustomEndpointConfig myconfig = (MyCustomEndpointConfig) config;
    myconfig.doThing();
} 

The EndpointConfig config represented here needs to show the truth of how the Connection/Session was created, not the limited scope of what the application's Configurator requested.

Using a param that is anything but EndpointConfig here is invalid.

@OnOpen
public void open(MyCustomEndpointConfig config, Session session) // <-- this is a DeploymentException (invalid params)
{
    config.doThing();
} 

Even using ServerEndpointConfig on your params is invalid, aka ...

@OnOpen
public void open(ServerEndpointConfig config, Session session) // <-- this is a DeploymentException (invalid params)
{
    MyCustomEndpointConfig myconfig = (MyCustomEndpointConfig) config;
} 

4. The javadoc of modifyHandshake does not inform the user he gets a wrapper instead of his implementation. The backward incompatible change must not be done in a minor release, it must have been done in major release of WebSeocket 3.0.

This is not a backward incompatible change.
In fact the original javadoc for that specifically says "the configuration object involved in the handshake", it doesn't say "the application provided ServerEndpointConfig".
This can be darn near anything, as long as it satisfies the ServerEndpointConfig interface it is valid.
This would be a dynamically created ServerEndpointConfig if using the @ServerEndpoint annotation too.

[joakime]

I'd prefer to avoid re-instating the grey area. The requirements of the Javaassist solution are not ideal. If we are thinking about WebSocket 3.0 as the solution then how about something like this:

• New class HandshakeServerEndpointConfig extends ServerEndpointConfig
• Wraps an existing ServerEndpointConfig
• Adds a new method to obtain the wrapped ServerEndpointConfig
• Overrides getUserProperties() to provide a per instance Map that is initially populated with a shallow copy of getUserProeprties() from the wrapped ServerEndpointConfig
• Add a new modifyHandshake() method that uses HandshakeServerEndpointConfig rather than ServerEndpointConfig
• Have the default implementation of that new method call the old modifyHandshake() method
• Deprecate the old modifyHandshake() method

The spec would be updated to state that the container wraps the original ServerEndpointConfig in an instance of HandshakeServerEndpointConfig before calling the new version of modifyHandshake().

The intention is that it is a NO-OP for existing code unless it needs to get hold of the original user properties map.

Now this would be a backward incompatible change. I'm -1 on this proposal as it stands.

This ability to unwrap cannot possible represent the truth of the ServerEndpointConfig object. (re prior statement about dynamic classes, or decorated classes, etc).
Access to the application provided ServerEndpointConfig is not a thing in the spec, it has never been part of the spec.
At best it was undefined. So going from undefined to defined is not a backward incompatible change. (eg: like when the Servlet spec declared Async, the default behavior on <async-supported> was undefined at first, some containers chose true, others chose false, a subsequent point release spec update defined it as false. Some containers had to hussle to fix this, but no container saw this change as a backward compatibility change)

The ServerEndpointConfig exists to register endpoint defaults.
Many of the methods on it are not dynamic, and you cannot change them once the endpoint is registered.

The methods that are accessed once, at initialization, not every time the endpoint upgrades.
These values never change for the lifetime of the registered websocket.

• ServerEndpointConfig.getEndpointClass()
• ServerEndpointConfig.getExtensions()
• ServerEndpointConfig.getPath()
• ServerEndpointConfig.getSubProtocols()
• ServerEndpointConfig.getEncoders()
• ServerEndpointConfig.getDecoders()

The methods that SHOULD be called per upgrade. (this keeps behavior with manual ServerEndpointConfig and @ServerEndpoint(config=<class>) annotation consistent)

• ServerEndpointConfig.getConfigurator()

The methods that MUST be called per upgrade. (this method/config does not exist if using the @ServerEndpoint annotation)
This per-endpoint instance behavior is part of jakartaee/websocket#235 (comment) (and it's impossible to implement the referenced HttpSession to endpoint instance technique without doing it this way. All other techniques are not thread safe)

• ServerEndpointConfig.getUserProperties()

Note that the websocket spec does not define how often the ServerEndpointConfig methods are called, or when in the lifecycle, etc.
All of that is undefined as far as the spec is concerned.

[jansupol]

@joakime Thank you for the elaborate about how Jetty works. However, I think not every statement you made is supported by the spec, though.

Now this would be a backward incompatible change. I'm -1 on this proposal as it stands.

I assume you mean this would be an incompatible change for Jetty if the modifyHandshake should contain the user-provided ServerEndpointConfig. In that case, you would be correct. But it is not the case, IMO. the modifyHandshake would be exactly the same as each implementation has it, no change, and there would be a new ModifyHandshake(HandshakeServerEndpointConfig ...) for which the only change Jetty would need to make would be for the ServerEndpointConfig wrapper class to implement HandshakeServerEndpointConfig.

Access to the application provided ServerEndpointConfig is not a thing in the spec, it has never been part of the spec.

It never has been said the user-provided ServerEndpointConfig must not be accessible, either. There are good reasons for the instance to be accessible:

1. The ServerApplicationConfig accepts an instance of ServerEndpointConfig, which is instantiated by the user. It is not a class to be instantiated (unlike in the annotated endpoint case) by the WebSocket implementation, and the user can expect the instance to be useable. The fact that the instance is registered to the ServerApplicationConfig suggests that the programmatic endpoints are dealt with a bit differently than in the annotated endpoint case.
2. The user methods in the ServerEndpointConfig may make more sense than to have the code directly in the ServerEndpointConfig.Configuration#modifyHandshake so that the methods may rely on the result of #getPath, getSubProtocols, etc.

This ability to unwrap cannot possible represent the truth of the ServerEndpointConfig object. (re prior statement about dynamic classes, or decorated classes, etc).

This statement about dynamic/decorated classes does not justify the reason to prohibit the user implementation, decorating the class, etc. is done at the registration point, not at the modifyHandshake time. The getEndpointClass() invoked on the user-provided ServerEndpointConfig will of course return the original unmodified/undecorated class.

Is there really some good reason to prohibit the user-defined ServerEndpointConfig be accessible at the modifyHandshake time?

[markt-asf]

I'd be happy with the spec as is but I can see the point that forcing the container to wrap the ServerEndpointConfig could break existing apps that assume the original instance is passed. My assumption is the JavaAssist type approach has too many additional requirements.
How about a simpler approach. The aim is to provide a way to expose the original ServerEndpointConfig. Something like:

• New class HandshakeServerEndpointConfig extends ServerEndpointConfig
• Wraps an existing ServerEndpointConfig
• Adds a new method to obtain the wrapped ServerEndpointConfig
• Overrides getUserProperties() to provide a per instance Map that is initially populated with a shallow copy of getUserProperties() from the wrapped ServerEndpointConfig

Apps that want the original ServerEndpointConfig would need to cast to HandshakeServerEndpointConfig and call the new method.

I think that could be argued as all clarification and therefore we'd stick with 2.1 as a version.

[joakime]

The ServerEndpointConfig.getEndpointClass() exists for multiple purposes.
First is to ensure that the class exists and is accessible to satisfy the DeploymentException requirements of the websocket spec.
Next is to provide the class type for when the eventual ServerEndpointConfig.Configurator.getEndpointInstance(Class<T> endpointClass) is called (per endpoint instance).

Lets talk about the call order for ServerEndpointConfig.Configurator.

The call order is (copied here from a previous talk about this, all the way back in 2013) ...

1. does the path match any of the ServerEndpointConfig.getPath() entries
   • If no match, return 404 to upgrade
2. pass upgrade request into ServerEndpointConfig.Configurator.checkOrigin()
   • If not valid, return error to upgrade response
   • create HandshakeResponse
3. pass upgrade request into ServerEndpointConfig.Configurator.getNegotiatedSubprotocol()
   • store answer in HandshakeResponse
4. pass upgrade request into ServerEndpointConfig.Configurator.getNegotiatedExtensions()
   • store answer in HandshakeResponse
5. Create new endpoint specific ServerEndpointConfig object. copy encoders, decoders, and User Properties. This new ServerEndpointConfig wraps default for path, extensions, endpoint class, subprotocols, configurator.
6. pass upgrade request, response, and new ServerEndpointConfig into ServerEndpointConfig.Configurator.modifyHandshake() - exception means fail the upgrade
7. call ServerEndpointConfig.getEndpointClass() to obtain the base class (for it's type info)
8. use class on ServerEndpointConfig.Configurator.getEndpointInstance(Class<T>) to obtain base endpoint instance.
9. create Session, associate endpoint instance and EndpointConfig object.
10. Inform endpoint instance of connect (per @onopen / Endpoint.onOpen as a result of the websocket RFC defined state change)
11. annotated methods that want EndpointConfig gets the one associated with this Session.
12. calls to Session.getUserProperties() returns EndpointConfig.getUserProperties()

To note, the ServerEndpointConfig.Configurator is a singleton, per mapped ServerContainer endpoint.

This is intentional, and desired, to allow implementors several features.

• to return the same Endpoint instance for multiple peers if they so desire. The so called stateless approach to websocket writing.
• to have a single point of management of expensive resources for all Endpoint instances

If the implementations created a new Configurator for every handshake, this technique would not be possible.

This behavior and call order is how Jetty, Tomcat, Wildfly, and Glassfish/Tyrus work today. (tested this myself).

If the ServerEndpointConfig was a singleton, instead of the ServerEndpointConfig.Configurator you would have a more gotchas for threading.

Take this for example ...

1. MyServerEndpointConfig msec = new MyServerEndpointConfig(); // singleton
2. Upgrade A (to /ws/foo)
3. (A) modifyHandshake(msec, req, resp)
4. (A) String fz = req.getHeader("X-Foo"))
5. Upgrade B (to /ws/foo)
6. (B) modifyHandshake(msec, req, resp)
7. (B) String fz = req.getHeader("X-Foo"))
8. (A) msec.getUserProperties().put("zed", fz); // <-- oops, we just put the wrong value.
9. (B) msec.getUserProperties().put("zed", fz);

If you want common behavior, and singleton style logic, the place for that is in the ServerEndpointConfig.Configurator, not the ServerEndpointConfig.
If you want to carry over information from during the upgrade to the websocket, the UserProperties is the way to do that. (this includes getting data from the Servlet / HTTP layer into websocket, custom objects, singleton sources, etc).
The UserProperties was designed to be a Map<String,Object> so you can get even complex objects out of it. (Similar to servlet attributes)

In the @OnOpen annotation only allows for EndpointConfig to be declared.
If your websocket declaration has anything but EndpointConfig it's a DeploymentException in the websocket spec.
There is no expectation of casting, never has been.

In fact, in the millions of installations, and years of experience with javax.websocket / jakarta.websocket, you are the only one to ever have asked about casting an EndpointConfig in onOpen or modifyHandshake to an application provided EndpointConfig.

• https://stackoverflow.com/search?q=javax.websocket+onopen+endpointconfig+cast
• https://stackoverflow.com/search?q=javax.websocket+modifyhandshake+endpointconfig+cast

(drop the +cast to see the topics related to above, but without cast in play)

Casting of the EndpointConfig would be a bad idea as well.
A library can ship an optional Endpoint class, and then ask the webapp to register it to suite their needs. (several chat and pub/sub libraries do this).
That means using ServerContainer.addEndpoint(ServerEndpointConfig) or the ServerApplicationConfig for the webapp, which can easily use a different ServerEndpointConfig than the Endpoint is expecting (for @OnOpen behavior).
There's nothing tying together the ServerEndpointConfig and ServerEndpointConfig.Configurator as well.
They are separate, you can have a custom ServerEndpointConfig.Configurator an default ServerEndpointConfig, or even a different ServerEndpointConfig.Configurator then the ServerEndpointConfig.getConfigurator() returns (several auditing and metrics libraries do this).

The only API that is guaranteed to work for modifyHandshake and onOpen are the jakarta.websocket.server.ServerEndpointConfig or jakarta.websocket.EndpointConfig APIs.

I spent the last hour going through github search results for https://github.com/search?p=1&q=ServerEndpointConfig+modifyHandshake&type=Code (stopped at page 60) looking at how people use modifyHandshake(). The most common things they use it for is getting the HttpSession to the websocket EndPoint via the userProperties, security (user id, authn, authz) gathering/storing in user-properties/failing the upgrade by throwing an Exception, and in a distant third collecting other odds and ends from the servlet/request layer to the endpoint via the user-properties. (found a few bugs in implementations too)

The one, and only one place, that I found with casting in onOpen isn't even code, but a discussion in an issue ...
jakartaee/websocket#115 - an issue from before websocket 1.0 and discussion on how the API should behave.

Simply put, being able to cast to the application (or server provided) EndpointConfig instance is new functionality and that represents a backward incompatible change.
A change that is simply not supported consistently by any existing websocket server container today, this change (to allow casting to application provided EndpointConfig).

[joakime]

I'd be happy with the spec as is but I can see the point that forcing the container to wrap the ServerEndpointConfig could break existing apps that assume the original instance is passed. My assumption is the JavaAssist type approach has too many additional requirements. How about a simpler approach. The aim is to provide a way to expose the original ServerEndpointConfig. Something like:

• New class HandshakeServerEndpointConfig extends ServerEndpointConfig
• Wraps an existing ServerEndpointConfig
• Adds a new method to obtain the wrapped ServerEndpointConfig
• Overrides getUserProperties() to provide a per instance Map that is initially populated with a shallow copy of getUserProperties() from the wrapped ServerEndpointConfig

Apps that want the original ServerEndpointConfig would need to cast to HandshakeServerEndpointConfig and call the new method.

I think that could be argued as all clarification and therefore we'd stick with 2.1 as a version.

This doesn't solve anything in my opinion.
You still have to provide a new ServerEndpointConfig with a new UserProperties instance before calling modifyHandshake anyway.
You can't just create a new version of the same class, that's silly.
Unwrapping is broken as it doesn't represent the "point in time" of the upgrade.

[joakime]

This proposed change also allows for the registered values on ServerEndpointConfig to change arbitrarily now.
What if the ServerEndpointConfig decides to change the returned values for .getEncoders(), or getProtocols() or getEndpointClass() at some point in time.
The ServerEndpointConfig that is passed into the onOpen / modifyHandshake methods need to have stability from the point in time it goes through the upgrade up until it enters the websocket endpoint.
This means we (the container implementors) have to wrap to store the "at time of upgrade" values to return when it's called again.
Imagine what would happen if we allowed ServerEndpointConfig.getPath() to change after registration (eek).

Lets look at the most common Configurator (found in hundreds of places on github, from as far back as 2014, with minor differences in null handling) ...

import jakarta.servlet.http.HttpSession;
import jakarta.websocket.HandshakeResponse;
import jakarta.websocket.server.HandshakeRequest;
import jakarta.websocket.server.ServerEndpointConfig;

public class HttpSessionConfigurator extends ServerEndpointConfig.Configurator {
    @Override
    public void modifyHandshake(ServerEndpointConfig sec, HandshakeRequest request, HandshakeResponse response) {
        HttpSession httpSession = (HttpSession) request.getHttpSession();
        if (httpSession != null) {
            sec.getUserProperties().put(HttpSession.class.getName(),httpSession);
        }
    }
}

If the ServerEndpointConfig is a single instance across all upgrades for that endpoint, or with a singleton UserProperties map, this wouldn't work.
What would Session.getUserProperties() return? A UserProperties that's unique per endpoint instance.
But if the ServerEndpointConfig is wrapped before calling it always works.
In this scenario, only the ServerEndpointConfig.Configurator is a single instance across all upgrade requests (as designed).

[joakime]

It helps to view ServerEndpointConfig as a registration record, not a live class.
It's the basis for how endpoints are registered, at registration time.
After that point in time (container init), all of the values are static/read-only/immutable.
It's the baseline for creating the UserProperties that is used in modifyHandshake() and eventually ends up in Session.getUserProperties().

The API should have been ...

Configurator.modifyHandshake(ServerEndpointConfig config, Map<String,Object> instanceUserProperties, HandshakeRequest request, HandshakeResponse response)
ServerContainer.addEndpoint(ServerEndpointConfig config, Map<String,Object> baseUserProperties)

Where the ServerEndpointConfig (actually EndpointConfig) has no getUserProperties() method.
If you wanted custom behavior in an instance of the Endpoint, you would instantiate the endpoint class yourself and populate it before returning it to the container (this even applies to the client usage of EndpointConfig)

But that ship sailed a long time ago, so we are stuck with wrapping of the EndpointConfig to support this extremely common use case (getting objects from http/upgrade scope to websocket scope).

[jansupol]

If the ServerEndpointConfig is a single instance across all upgrades for that endpoint, or with a singleton UserProperties map, this wouldn't work.
What would Session.getUserProperties() return? A UserProperties that's unique per endpoint instance.

Now you are right. Without wrapping, Session.getUserProperties() would have concurrency issues. Tyrus always used the user-provided instance, but that could never be working properly.
@joakime Great discussion, thank you.
@markt-asf Thank you for the provided workarounds. I would still prefer those new methods you came up with since those are better explanatory, but I assume they are not necessary.

[jansupol]

I am reopening the issue. The TCK tests have and always had a test com/sun/ts/tests/websocket/ee/jakarta/websocket/server/serverendpointconfig/configurator/WSCClient.java#modifyHandshakeConfigTest. This test has:

public class ModifyHandshakeConfigurator extends Configurator {
  private static ServerEndpointConfig config;
  
  private static void setConfig(ServerEndpointConfig config) {
    ModifyHandshakeConfigurator.config = config;
  }

  public void modifyHandshake(ServerEndpointConfig sec, HandshakeRequest request, HandshakeResponse response) {
    ModifyHandshakeConfigurator.setConfig(sec);
    ....
    super.modifyHandshake(sec, request, response);
  }
}

And the endpoint:

@ServerEndpoint(value = "/modifyhandshake", configurator = ModifyHandshakeConfigurator.class)
public class WSCModifyHandshakeServer {

  ServerEndpointConfig config;

  @OnMessage
  public String onMessage(String msg) {
    ....
    if (msg.equals("config"))
      ret = ModifyHandshakeConfigurator.getConfig().getClass().getName()
          .equals(config.getClass().getName());
    }
    return String.valueOf(ret);
  }

  @OnOpen
  public void onOpen(EndpointConfig config) {
    this.config = (ServerEndpointConfig) config;
  }

1. The test checks the configurator is the same class
2. The test is casting the EndpointConfig to ServerEndpointConfig
3. @joakime How did Jetty pass the TCK tests previously?
4. Wrapping the ServerEndpointConfig class is a backward-incompatible change.

[markt-asf]

The test isn't doing what you think it is doing.

The test is confirming that the EndpointConfig instance passed to modifyHandshake() is of the same class as the EndpointConfig instance passed to a method annotated with @OnOpen that includes a EndpointConfig parameter.

The associated assertion makes no reference to the original EndpointConfig being passed.

[jansupol]

Right, the wrapper needs to be sent to the onOpen...

[joakime]

That (the apparent casting in onOpen) has never been supported by Jetty.
The prior TCK (before going to Jakarta) did not appear to have this test case.
Jetty only had issues on that TCK related to vaguely defined encoding/decoding specs in jakarta websocket (init/destroy timing issues), and stream based read behavior when receiving multiple frames in a single read (threading related, it had flaky results).

The flaws in the 2 pieces of code you pasted ...

1. Casting in onOpen (never supported, never part of the spec, the TCK cannot invent new rules here)
2. Being an annotated endpoint, the declared Configurator is static (the configurator code seems to assumes a new Configurator every upgrade).
3. A configurator that holds a value from a single upgrade to use in the post-upgraded endpoint. (the static config field is a threading issue)

That TCK testcase is not a real world use case.

NOT wrapping the ServerEndpointConfig is THE backward incompatible change and will surprise countless existing use cases (the UserProperties unique to each server endpoint upgrade).
There's no other way, with the existing API, to accomplish that.
You have to wrap to present a unique UserProperties for modifyHandshake to make sense.
If you unwrap before calling onOpen then you lose the changes in UserProperties.

Changing the rules now will cause massive upheaval in the many existing projects and libraries that use jakarta websockets today. (I'll be happy to show you a few thousand examples across github if you want to see them).

If you want that (casting in onopen), then the API has to undergo some serious changes to support UserProperties per upgrade. (too late to do that now)

Now for point 2, the expectation that the application declared ServerEndpointConfig class or instance will be used and presented as-is also doesn't take into account other real world use cases (eg: proxy classes, mutated classed, decorated classes, class file transformers, various forms of instrumentation, etc) the only API guarantee is what is spec'd by the jakarta websocket spec, which is the EndpointConfig and/or ServerEndpointConfig interface. Any other use case is out of scope of the spec and is implementation specific.

Your implementation can choose to use casting and completely be incompatible with the past 8 years of real world use cases.
But that is not a rule (casting as a requirement) for the websocket spec, and shouldn't be part of the spec.

Now, that being said, the new rule on this new version of the websocket spec is that all UserProperties are per endpoint instance.
That new rule is what you need to support.

Lets take these 2 proposed use cases.

Use Case 1: Getting the HttpSession of the upgrade into the Endpoint via UserProperties

Note: There are thousands of examples of this use case on github.

import jakarta.servlet.http.HttpSession;
import jakarta.websocket.HandshakeResponse;
import jakarta.websocket.server.HandshakeRequest;
import jakarta.websocket.server.ServerEndpointConfig;

public class HttpSessionConfigurator extends ServerEndpointConfig.Configurator {
    @Override
    public void modifyHandshake(ServerEndpointConfig sec, HandshakeRequest request, HandshakeResponse response) {
        HttpSession httpSession = (HttpSession) request.getHttpSession();
        if (httpSession != null) {
            sec.getUserProperties().put(HttpSession.class.getName(),httpSession);
        }
    }
}

@ServerEndpoint(value = "/wants-session", configurator = HttpSessionConfigurator.class)
public class MyEndpoint {
  private HttpSession httpSession;
  @OnOpen
  public void onOpen(EndpointConfig config) {
    this.httpSession = (HttpSession) config.getUserProperties().get(HttpSession.class.getName());
  }
}

Use Case 2: Getting the HttpSession of the upgrade into the Endpoint via custom EndpointConfig

import jakarta.servlet.http.HttpSession;
import jakarta.websocket.HandshakeResponse;
import jakarta.websocket.server.HandshakeRequest;
import jakarta.websocket.server.ServerEndpointConfig;

public class HttpSessionServerEndpointConfig extends ServerEndpointConfig {
    private HttpSession httpSession;
    public void setHttpSession(HttpSession httpSession) {
        this.httpSession = httpSession;   
    }
    public HttpSession getHttpSession() {
        return this.httpSession;
    }
}

public class HttpSessionConfigurator extends ServerEndpointConfig.Configurator {
    @Override
    public void modifyHandshake(ServerEndpointConfig sec, HandshakeRequest request, HandshakeResponse response) {
        HttpSessionServerEndpointConfig config = (HttpSessionServerEndpointConfig) sec;
        HttpSession httpSession = (HttpSession) request.getHttpSession();
        config.setHttpSession(httpSession);
    }
}


public class MyEndpoint extends Endpoint {
    private HttpSessionServerEndpointConfig config;
    private HttpSession httpSession;
    public void onOpen(Session session, EndpointConfig config) {
        this.config = (HttpSessionServerEndpointConfig) config;
        this.httpSession = this.config.getHttpSession();
  }
}

Use Case 2 cannot work when the application provides a ServerEndpointConfig.

• The obvious threading / concurrency issues.
• The container cannot create a new HttpSessionServerEndpointConfig
• The jakarta websocket spec has never had this ability
• The jakarta websocket spec doesn't mandate that a ServerEndpointConfig has
  a default constructor that the container can use to create a new ServerEndpointConfig
  (The spec should probably have had a mandated constructor that passes in a UserProperties that the ServerEndpointConfig must use, but that's just messy)
• There's nothing tieing together the modifyHandshake call and the onOpen call,
  meaning there's no possible way to carry over information from a specific
  upgrade to an endpoint.
  (Both the modifyHandshake and onOpen need something to identify that they are related, the Session.getId()?)
• annotated endpoints cannot use this technique

Use case 1 is how the real world has implemented this carryover of information
from the upgrade into the endpoint.

OK, I've presented the 2 use cases being disucussed here.

Now, present a way to carry over information from the upgrade to the endpoint.

We can safely assume that if a ServerEndpointConfig instance is provided by the application then the application does not want the container to recreate it.

The jakarta websocket spec defined class for handling common upgrade behavior, and
the recommended place for holding application specific objects / state, that are useful
for the application, is the ServerEndpointConfig.Configurator.

The jakarata websocket spec has always had language that the ServerEndpointConfig is about
configuration of the endpoint, and the configurator is about controlling the upgrade/instance.

See:

• Section 2.1.1 Endpoint Lifecycle
• Section 2.1.2 Sessions
• Section 4.1.5 @ServerEndpoint configurator

[markt-asf]

Please close this challenge as invalid.

[scottmarlow]

As per Websocket Specification team decision as determined by Spec lead @markt-asf this challenge is now being closed as invalid.

[jansupol]

@markt-asf Thank you for pointing out wrapping the class on @OnOpen
@joakime Thank you for a large elaborate.

The prior TCK (before going to Jakarta) did not appear to have this test case.

WebSocket 1.1 was the last TCK that added new tests (before jakarta). That was before Java EE 8. This test is there for WebSocket 1.1 TCK, Java EE 8 TCK, Jakarta EE 8 TCK, and Jakarta EE 9 TCK. Sure, it is possible the test is not correct.

Casting in onOpen (never supported, never part of the spec, the TCK cannot invent new rules here)

This is likely for a broader discussion. WebSocket Spec never said it is not supported. There is no reason for that not being supported. The wrapper passed in is ServerEndpointConfig in the end. I do not see a reason for not allowing the user to be able to call the ServerEndpointConfig methods such as getPath().

Now, that being said, the new rule on this new version of the websocket spec is that all UserProperties are per endpoint instance.
That new rule is what you need to support.

I agree with that. I am not 100% happy about how it is done, but let's agree on wrapping.

For Use Case 2:
I agree with you that the use-case should not be required by the spec/tck test.

There's nothing tieing together the modifyHandshake call and the onOpen call,
meaning there's no possible way to carry over information from a specific
upgrade to an endpoint.
(Both the modifyHandshake and onOpen need something to identify that they are related, the Session.getId()?)

I believe these are called within a single thread defined by the HttpRequest/HttpSession.

[jansupol]

I agree that the test challenge is invalid, thank you again @markt-asf @joakime @scottmarlow