I am confused though - I reviewed the robaho source code which came from the
JDK, and the setAttribute is on the exchange, which is per request, not per
context.
> On Dec 6, 2024, at 8:41 AM, robert engels <reng...@ix.netcom.com> wrote:
>
> 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
>>>>>>>
>
