In minor defense of the JDK (I am worried more about ambiguity) - I’ve seen the
JDK users implement this by add to the request headers - but with the JDK
making this read-only, this is no longer possible, so the get/set attribute is
the only viable way to pass per request information between layers.
> On Dec 5, 2024, at 7:27 PM, Ethan McCue <et...@mccue.dev> wrote:
>
> After mulling it over some more, I think that as is there is actually no
> valid use for .setAttribute as implemented by the JDK
>
> Even the most trivial usages of it are broken under moderate load. This
> includes the usage in SimpleFileServer.createOutputFilter and
> SimpleFileServer.createFileHandler
>
> import com.sun.net.httpserver.HttpServer;
> import com.sun.net.httpserver.SimpleFileServer;
>
> import java.io.ByteArrayOutputStream;
> import java.net.InetSocketAddress;
> import java.net.URI;
> import java.net.http.HttpClient;
> import java.net.http.HttpRequest;
> import java.net.http.HttpResponse;
> import java.nio.charset.StandardCharsets;
> import java.nio.file.Files;
> import java.nio.file.Path;
> import java.util.concurrent.Executors;
> import java.util.concurrent.atomic.AtomicInteger;
> import java.util.regex.Pattern;
>
> public class Main {
> public static void main(String[] args) throws Exception {
> Files.writeString(Path.of("./a"), "a".repeat(100000));
> Files.writeString(Path.of("./b"), "b".repeat(100000));
>
> var serverExecutor = Executors.newFixedThreadPool(2);
> var baos = new ByteArrayOutputStream();
> var server = HttpServer.create(new InetSocketAddress(8841), 0);
> server.createContext("/",
> SimpleFileServer.createFileHandler(Path.of(".").toAbsolutePath()))
> .getFilters().add(SimpleFileServer.createOutputFilter(baos,
> SimpleFileServer.OutputLevel.VERBOSE));
> server.setExecutor(serverExecutor);
> server.start();
>
> Thread.sleep(1000);
>
> var executor = Executors.newFixedThreadPool(2);
>
> int total = 10000;
>
> AtomicInteger failures = new AtomicInteger();
> for (int i = 0; i < total; i++) {
> String file = i % 2 == 0 ? "a" : "b";
> executor.submit(() -> {
> try {
> var client = HttpClient.newHttpClient();
> client.send(
>
> HttpRequest.newBuilder(URI.create("http://0.0.0.0:8841/"; + file)).build(),
> HttpResponse.BodyHandlers.ofString()
> );
> } catch (Exception e) {
> e.printStackTrace(System.out);
> failures.getAndIncrement();
> }
> return null;
> });
> }
>
> executor.close();
>
> int a = 0;
> int b = 0;
> var s = baos.toString(StandardCharsets.UTF_8).split("\n");
> for (var line : s) {
> Pattern aPattern = Pattern.compile("Resource requested: (.+)/a");
> Pattern bPattern = Pattern.compile("Resource requested: (.+)/b");
> if (aPattern.matcher(line).find()) {
> a++;
> }
> else if (bPattern.matcher(line).find()) {
> b++;
> }
> }
> System.out.println("Reported a request to /a: " + a);
> System.out.println("Reported a request to /b: " + b);
> System.out.println("Failures: " + failures);
>
> server.stop(0);
> serverExecutor.close();
> }
> }
>
> Despite an equal number of requests being made to /a and /b the output filter
> will report a randomly diverging amount. This is because there is simply no
> way to avoid concurrent requests clobbering each-others state while calling
> setAttribute on an exchange does not actually set an attribute on that
> exchange.
>
> On Thu, Dec 5, 2024 at 11:15 PM Ethan McCue <et...@mccue.dev
> <mailto:et...@mccue.dev>> wrote:
>> Sorry, meant to send this to the list:
>>
>> I will add as maybe obvious context that the way the JDK currently
>> implements this is (I think, correct me if I am wrong) a security nightmare.
>> That it might not be obvious (or uniform across an ecosystem of
>> implementations) that exchange.setAttribute("CURRENTLY_AUTHENTICATED_USER",
>> "..."); will not actually be setting an attribute on the exchange, but
>> instead a global thing, is an issue
>>
>> If this is a deliberate choice in the existing implementation, I am curious
>> to know how it came about.
>>
>> On Thu, Dec 5, 2024 at 11:13 PM Robert Engels <reng...@ix.netcom.com
>> <mailto:reng...@ix.netcom.com>> wrote:
>>> Hi,
>>>
>>> I read the bug report. I don’t think this is sufficient. This is a
>>> specification so in order to have compliant behavior no matter the
>>> implementation there cannot be a different set of rules for the reference
>>> implementation vs others.
>>>
>>> So the api should be clarified in a non ambiguous manner and then one or
>>> more implementations can be classified as non compliant.
>>>
>>> Robert
>>>
>>>> On Dec 5, 2024, at 6:31 AM, Jaikiran Pai <jai.forums2...@gmail.com
>>>> <mailto:jai.forums2...@gmail.com>> wrote:
>>>>
>>>>
>>>> Hello Ethan,
>>>>
>>>> Thank you for noticing this and bringing this up here. I've raised
>>>> https://bugs.openjdk.org/browse/JDK-8345577 and we will address this
>>>> shortly.
>>>>
>>>> -Jaikiran
>>>>
>>>> On 05/12/24 3:22 am, Ethan McCue wrote:
>>>>> Sorry
>>>>>
>>>>> Before:
>>>>>
>>>>> * {@link Filter} modules may store arbitrary objects with {@code
>>>>> HttpExchange}
>>>>> * instances as an out-of-band communication mechanism. Other filters
>>>>> * or the exchange handler may then access these objects.
>>>>>
>>>>> Bungled the copy-paste
>>>>>
>>>>> On Thu, Dec 5, 2024 at 6:49 AM Ethan McCue <et...@mccue.dev
>>>>> <mailto:et...@mccue.dev>> wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> This change
>>>>>> (https://github.com/openjdk/jdk/commit/40ae4699622cca72830acd146b7b5c4efd5a43ec)
>>>>>> makes the Jetty implementation of the SPI be no longer fit the Javadoc.
>>>>>>
>>>>>> HttpContexts are not per-request while the previous Javadoc implied that
>>>>>> the attribute mechanism on exchanges was.
>>>>>>
>>>>>> Before:
>>>>>>
>>>>>> * Sets an attribute with the given {@code name} and {@code value}
>>>>>> in this exchange's
>>>>>> * {@linkplain HttpContext#getAttributes() context attributes}.
>>>>>> * or the exchange handler may then access these objects.
>>>>>>
>>>>>> After:
>>>>>>
>>>>>> * Sets an attribute with the given {@code name} and {@code value}
>>>>>> in this exchange's
>>>>>> * {@linkplain HttpContext#getAttributes() context attributes}.
>>>>>> *
>>>>>> * @apiNote {@link Filter} modules may store arbitrary objects as
>>>>>> attributes through
>>>>>> * {@code HttpExchange} instances as an out-of-band communication
>>>>>> mechanism. Other filters
>>>>>> * or the exchange handler may then access these objects.
>>>>>>
>>>>>> The Jetty implementation, I think rightfully, assumed that this context
>>>>>> was per-request and implemented it as so.
>>>>>>
>>>>>> https://github.com/jetty/jetty.project/blob/jetty-12.0.x/jetty-core/jetty-http-spi/src/main/java/org/eclipse/jetty/http/spi/JettyHttpExchangeDelegate.java#L223
>>>>>>
>>>>>> As such, I don't think simply a javadoc change as a resolution to these
>>>>>> issues is applicable
>>>>>>
>>>>>> https://bugs.openjdk.org/browse/JDK-8345233
>>>>>> https://bugs.openjdk.org/browse/JDK-8235786
>>>>>>
