Skip to content

Conversation

@franciscojmc
Copy link

@franciscojmc franciscojmc commented Feb 27, 2025

The SERVER_PORT is optional but must not be set if it is not present.

Fixes socketry/falcon#118.

Types of Changes

  • Bug fix.

Contribution

end

# SERVER_PORT should not be set if it is nil
env.delete(CGI::SERVER_PORT) if env[CGI::SERVER_PORT].nil?
Copy link
Author

Choose a reason for hiding this comment

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

I opted to go with the delete approach to avoid repeating the same logic in each rack version adapter.

Copy link
Member

Choose a reason for hiding this comment

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

This low level adapter is fairly performance critical. In this case, we have 3 hash operations, (1) an assignment of SERVER_PORT to nil, then an unconditional lookup of SERVER_PORT, and a delete operation. I think it's okay to duplicate the code for each adapter to ensure that on the fast path, no hash operations are involved, so I'd prefer if we have something like:

if server_port
  env[CGI::SERVER_PORT] = server_port
end

Duplicating the code between the adaptors is okay in this case, since in my mind they are independent specifications (even if they are the same in this regard). Does that seem reasonable?

Copy link
Author

Choose a reason for hiding this comment

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

Absolutely. Considering the performance-critical nature of this code path, I agree with all your points. Will update the implementation accordingly.

@franciscojmc franciscojmc marked this pull request as ready for review February 28, 2025 19:02
@ioquatix ioquatix merged commit 5e426e6 into socketry:main Feb 28, 2025
33 of 38 checks passed
@ioquatix
Copy link
Member

Thanks for your contribution.

@ioquatix ioquatix added the bug Something isn't working label Feb 28, 2025
@ioquatix ioquatix self-assigned this Feb 28, 2025
@ioquatix ioquatix added this to the v0.11.2 milestone Feb 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rack::Lint::LintError occurs when Host: has no port number

2 participants