Sorry, that was incorrect - the api call is on the exchange, but it get/sets
the attributes on the related context.
This seems wrong to me as well, and I’ll probably change it in the robaho
version, but this would be a breaking change with the JDK implementation.
I agree with Ethan, that I think this should be fixed in the JDK, or at least
be marked that the JDK impl is non-compliant.
> On Dec 6, 2024, at 9:15 AM, robert engels <reng...@ix.netcom.com> wrote:
>
> 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
>>>>>>>>
>>
>
