Javadoc change in HttpExchange makes external implementations non-complaint

[Ethan McCue]

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 <ethan at 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 <rengels at 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.forums2013 at 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 <ethan at 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
>>>
>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/net-dev/attachments/20241206/ee5d24c5/attachment-0001.htm>