athena-framework / athena

An ecosystem of reusable, independent components
https://athenaframework.org
MIT License
211 stars 17 forks source link

Athena Regex path forces JIT when JIT is not available #380

Closed skinnyjames closed 7 months ago

skinnyjames commented 7 months ago

I think Athena controllers return 500 when resolving routes when using PCRE2 without JIT support.

2024-03-31T15:10:02.191054Z  ERROR - athena.framework: Uncaught exception #<Regex::Error:Regex match error: bad JIT option> at lib/athena-routing/src/ext/regex.cr:61:9 in 'match_data'
Regex match error: bad JIT option (Regex::Error)
  from lib/athena-routing/src/ext/regex.cr:61:9 in 'match_data'
  from lib/athena-routing/src/ext/regex.cr:69:18 in 'match_impl'
  from /Users/skinnyjames/.asdf/installs/crystal/1.11.2/src/regex.cr:614:12 in 'match_at_byte_index'
  from /Users/skinnyjames/.asdf/installs/crystal/1.11.2/src/regex.cr:563:12 in 'match'
  from lib/athena-routing/src/matcher/url_matcher.cr:141:21 in 'do_match'
  from lib/athena-routing/src/matcher/url_matcher.cr:33:8 in 'match'

Crystal Regex checks the return from jit_compile but I don't think it is being checked in the regex ext for athena routing.

https://github.com/athena-framework/athena/blob/master/src/components/routing/src/ext/regex.cr#L46

Fix / workaround is to check the result from jit_compile


    # CUSTOMIZE - Leverage JIT mode if possible
    match_count = if @force_jit && @jit
                    LibPCRE2.jit_match(@re, str, str.bytesize, byte_index, pcre2_match_options(options), match_data, PCRE2.match_context)
                  else
                    LibPCRE2.match(@re, str, str.bytesize, byte_index, pcre2_match_options(options), match_data, PCRE2.match_context)
                  end      
Blacksmoke16 commented 7 months ago

I think this is related to https://github.com/crystal-lang/crystal/issues/13150 in that by default I've been forcing the fast path API. However in this case it seems JIT mode isn't available, which is handled by the stdlib implementation, but because the extension is forcing it, we get the error.

Should be easy enough to fix.