pac-interpreter: Initial implementation #1
Conversation
jbaldassari
changed the title
|
|
||
| - name: Build and test | ||
| run: ./gradlew test --console plain | ||
|
|
There was a problem hiding this comment.
do we want a step to publish the test results as an artifact?
| * @return the result of executing the PAC script with the given URL and host. | ||
| * @throws PacInterpreterException if an error occurs evaluating the PAC script or parsing the results. | ||
| */ | ||
| FindProxyResult findProxyForUrl(final String url, final String host) throws PacInterpreterException; |
There was a problem hiding this comment.
So does this still take the full url in the first argument or is it a relative url?
There was a problem hiding this comment.
First argument is the full URL. Second argument is just the hostname (without port). It seems strange, but that's the contract of the FindProxyForURL function in the PAC spec, so it made sense to mirror that.
| } catch (IOException e) { | ||
| // This file is included in the jar, so if we can't open/read it something is seriously wrong. | ||
| // There is likely nothing the caller can do to handle this, so just rethrow as a runtime exception. | ||
| throw new RuntimePacInterpreterException(String.format("Failed to read \"%s\" from classpath", PAC_UTILS_PATH), e); |
There was a problem hiding this comment.
If it's fatal and the user can't do anything should it be an Error instead?
There was a problem hiding this comment.
You mean an exception that extends java.lang.Error instead of java.lang.RuntimeException? I could do that. There's a java.io.IOError that might make sense here.
| return Engine.newBuilder() | ||
| // Silence GraalVM warnings | ||
| // See: https://www.graalvm.org/latest/reference-manual/js/FAQ/#warning-implementation-does-not-support-runtime-compilation | ||
| .option("engine.WarnInterpreterOnly", "false") |
There was a problem hiding this comment.
looks like there is a performance hit but I assume we are ok with it since the pac interpreter shuold be realtively compute lite?
There was a problem hiding this comment.
Yes, exactly, and since we'll be caching the PAC itself, I think it won't be too bad. I've looked into how to get this working properly with the Graal JIT, but it's quite complicated. When I get to integrating this into the Link Agent I might see if I can get it working. Now that I'm thinking about it though, I might not want to disable this message permanently. Maybe I'll just document how to disable it via system properties instead.
| * James Baldassari <[email protected]> 07/14/2023 | ||
| */ | ||
|
|
||
| function dnsDomainIs(host, domain) { |
There was a problem hiding this comment.
Any specific changes worth reviewing here. I don't feel it's necesary to review the source code but if there are any deltas I can give them a look over...
There was a problem hiding this comment.
Nope, this is all standard stuff straight from Mozilla. The only changes are my two functions at the bottom that use the Graal-JVM integration to do things that JavaScript can't. But those are almost trivial implementations.
…tation on how to silence them if desired
…solvedProxyAddress()
Initial implementation of the PAC interpreter. See README for detailed documentation.
Artifacts are published to the Sonatype repository from which they can be released to Maven Central.