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 >>>>>>