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

Grizzly server filter does not take into account multiple contexts #737

Closed
zenbones opened this issue Jul 30, 2021 · 8 comments · Fixed by #834
Closed

Grizzly server filter does not take into account multiple contexts #737

zenbones opened this issue Jul 30, 2021 · 8 comments · Fixed by #834

Comments

@zenbones
Copy link

zenbones commented Jul 30, 2021

In the same Grizzly instance one may have more than one Tyrus server container because you are processing websockets requests on more than one application context / context path. Therefore, in the filter chain will be more than one filter, each associated with a different context path. The handleHandshake() code recognizes this...

    if (!upgradeRequest.getRequestURI().getPath().startsWith(contextPath)) {
      // the request is not for the deployed application
      return ctx.getInvokeAction();
    }

However, by this time the attribute holder for the filter context would already have been set true...

      final String ATTR_NAME = "org.glassfish.tyrus.container.grizzly.WebSocketFilter.HANDSHAKE_PROCESSED";

      final AttributeHolder attributeHolder = ctx.getAttributes();
      if (attributeHolder != null) {
        final Object attribute = attributeHolder.getAttribute(ATTR_NAME);
        if (attribute != null) {
          // handshake was already performed on this context.
          return ctx.getInvokeAction();
        } else {
          attributeHolder.setAttribute(ATTR_NAME, true);
        }
      }
      // Handle handshake
      return handleHandshake(ctx, message);

..and the next Tyrus filter in the chain will skip and simply invoke the next action. The problem is that...

attributeHolder.setAttribute(ATTR_NAME, true);

...fails to capture the reality of multiple contexts. Rather than a boolean, this should hold the context path of the filter, or a set of context paths of all Tyrus filters tried thus far, and if the current filter's context path is in the set, skip to the next action as the current code does, but, if not, try the handshake.

@zenbones
Copy link
Author

Here's a fix with the proper behavior...

      final String ATTR_NAME = "org.glassfish.tyrus.container.grizzly.WebSocketFilter.HANDSHAKE_PROCESSED";
      final AttributeHolder attributeHolder = ctx.getAttributes();

      if (attributeHolder != null) {

        final Object attribute = attributeHolder.getAttribute(ATTR_NAME);

        if (!(attribute instanceof Set)) {

          HashSet<String> contextPathSet = new HashSet<>();

          contextPathSet.add(contextPath);
          attributeHolder.setAttribute(ATTR_NAME, contextPathSet);
        } else {

          Set<String> contextPathSet = (Set<String>)attribute;

          if (!contextPathSet.add(contextPath)) {
            return ctx.getInvokeAction();
          } else {
            attributeHolder.setAttribute(ATTR_NAME, contextPathSet);
          }
        }
      }

      // Handle handshake
      return handleHandshake(ctx, message);

@zenbones
Copy link
Author

Year and half..... no one wants to leap on this huh?

@jansupol
Copy link
Contributor

Uhm...sometimes it is better to push us more often...sorry. Will try to get to this soon.

@jansupol
Copy link
Contributor

jansupol commented Feb 1, 2023

So basically if the attribute name would be different for each contextPath:

final String ATTR_NAME = "org.glassfish.tyrus.container.grizzly.WebSocketFilter.HANDSHAKE_PROCESSED." + contextPath;   

Then the rest of the code could remain as simple is, correct?

@jansupol
Copy link
Contributor

jansupol commented Feb 2, 2023

@zenbones

@zenbones
Copy link
Author

Sorry. I don't know if your fix is good. I know the one I posted works because we replace the class and use ours instead.

@zenbones
Copy link
Author

But hopefully you tested... We will eventually.

@jansupol
Copy link
Contributor

@zenbones I tried to test it. Let us know when any issues. Currently, the fix is in 2.1.3.

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 a pull request may close this issue.

2 participants