Skip to content

Review Handler Collection logic around InvocationType #11425

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

Open
joakime opened this issue Feb 20, 2024 · 3 comments
Open

Review Handler Collection logic around InvocationType #11425

joakime opened this issue Feb 20, 2024 · 3 comments
Assignees
Labels
Bug For general bugs on Jetty side

Comments

@joakime
Copy link
Contributor

joakime commented Feb 20, 2024

Jetty version(s)
12.0.6

Jetty Environment
Any

Java version/vendor (use: java -version)
Any

OS type/version
Any

Description
While reviewing PR #11412 a question about the InvocationType logic in the various Handler Collection implementations was brought up.

See

  • Handler.Sequence.setHandlers()
  • PathMappingsHandler.addMapping()
  • Handler.Singleton.updateHandler()
  • HotSwapHandler.setHandler()
  • WebSocketUpgradeHandler
@gregw
Copy link
Contributor

gregw commented Oct 2, 2024

see also #12266

@sbordet sbordet moved this to 🏗 In progress in Jetty 12.1.0 Jan 15, 2025
@sbordet sbordet unassigned gregw and lorban Jan 15, 2025
@gregw
Copy link
Contributor

gregw commented Mar 26, 2025

@joakime can you describe a bit more what this issue is about? If I look at Handler.Sequence I see:

        @Override
        public InvocationType getInvocationType()
        {
            if (isDynamic())
                return InvocationType.BLOCKING;

            InvocationType invocationType = InvocationType.NON_BLOCKING;
            for (Handler handler : _handlers)
                invocationType = Invocable.combine(invocationType, handler.getInvocationType());
            return invocationType;
        }

Which is not very efficient, but appears to be correct. So is this issue about efficiency or correctness?

@joakime
Copy link
Contributor Author

joakime commented Mar 26, 2025

@joakime can you describe a bit more what this issue is about? If I look at Handler.Sequence I see:

    @Override
    public InvocationType getInvocationType()
    {
        if (isDynamic())
            return InvocationType.BLOCKING;

        InvocationType invocationType = InvocationType.NON_BLOCKING;
        for (Handler handler : _handlers)
            invocationType = Invocable.combine(invocationType, handler.getInvocationType());
        return invocationType;
    }

Which is not very efficient, but appears to be correct. So is this issue about efficiency or correctness?

I filed this on behalf of @sbordet while we were reviewing PR #11412

It started as a result of trying to understand the lines ...

https://github.com/jetty/jetty.project/pull/11412/files#diff-e0bc618a8172099d95fe05f49ee72348860104e3d6b5c5048731b39640d1f79dR82-R87

And how they interacted with the various Handler collections behaviors too.
IIRC, Simone wanted there to be only 1 place to handle InvocationType, the handler itself, not the implementations that extend from the handlers.

@gregw gregw self-assigned this Mar 26, 2025
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
Status: 🏗 In progress
Development

No branches or pull requests

4 participants