On Tue, 22 Apr 2025 18:18:51 GMT, Daniel Fuchs <dfu...@openjdk.org> wrote:
>> src/java.net.http/share/classes/java/net/http/HttpRequest.java line 357:
>>
>>> 355: * @since TBD
>>> 356: */
>>> 357: public default <T> Builder setOption(HttpRequestOption<T>
>>> option, T value) { return this; }
>>
>> Bikeshed: we usually omit the set prefix for builders.
>
> I actually hesitated quite a bit on this one. We have a precedent with
> `Builder.setHeader(name, value)` - where the `setHeader` method replaces any
> value associated with the name with the given value, while `header(name,
> value)` would just add another value to the list of values associated with
> the name. I didn't want to give the impression that you could provide several
> values to the same option - and thus opted for naming that method
> `setOption`. I could let me be convinced to rename it if there is a strong
> concensus.
Didn't notice we already use `set` prefix in this builder. All good;
consistency is more important.
>> src/java.net.http/share/classes/java/net/http/HttpResponse.java line 937:
>>
>>> 935: HttpRequest initiatingRequest,
>>> 936: HttpRequest pushPromiseRequest,
>>> 937: PushId pushid,
>>
>> (Warning: I know little about HTTP, my assumptions and thus whole argument
>> may be invalid below!)
>>
>> This feels like users need to use push ids as units of synchronization on
>> this handler, such as tracking this with a ConcurrentHashMap.
>>
>> Is it possible for users to instead provide a `Function<PushId,
>> PushPromiseHandler>` in `HttpClient::sendAsync`? I think this might make it
>> easier for users as they no longer need to think about concurrency for
>> different ids.
>
> PushId is an HTTP/3 concept. It would be strange to have a generic method
> (sendAsync) that you could call with any requests, but with a parameter that
> could only be used for HTTP/3. We don't have pushIds for HTTP/2. I don't
> think we would want to take this route.
Do users need to track the push ids in a thread-safe data structure like
ConcurrentHashMap in a PushPromiseHandler?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24751#discussion_r2054677703
PR Review Comment: https://git.openjdk.org/jdk/pull/24751#discussion_r2054679688
