Re: [patch] JDK-4906983
On 8 Sep 2015, at 05:56, David Holmes wrote: > Hi Sebastian, > > On 8/09/2015 1:54 PM, Sebastian Sickelmann wrote: >> Hi, >> >> please find the link to the webrev[1] hosted at my dropbox in my first mail >> in this thread at the buttom of this mail. >> >> A direkt link to the including patch file can be found here: >> https://dl.dropboxusercontent.com/u/43692695/oss-patches/openjdk/JDK-4906983/jdk.patch > > Another rule is that all submitted patches must come in via OpenJDK > infrustructure so they fall under the term-of-use [1] - either within the > email to the list (though attachments get stripped often), or hosted on > cr.openjdk.java.net. An OpenJDK Author can host the patch on > cr.openjdk.java.net on your behalf. I have not looked at the proposed change, but if it is small just paste the patch into mail. Thanks, -Chris. > Cheers, > David > > [1] http://openjdk.java.net/legal/tou/terms > > >> -- Sebastian >> >> Am 08.09.2015 2:03 vorm. schrieb David Holmes : >>> >>> On 7/09/2015 11:46 PM, Ivan Krylov wrote: Hi Sebastian, The current OpenJDK rules [1] do not allow to accept patches from people that are not OCA signatories. >>> >>> For the record, small patches can be accepted: >>> >>> "A Participant is an individual who has subscribed to one or more >>> OpenJDK mailing lists. A Participant may post messages to a list, submit >>> simple patches, and make other kinds of small contributions. >>> >>> A Contributor is a Participant who has signed the Oracle Contributor >>> Agreement (OCA), ..." >>> >>> --- >>> >>> I did not see the referenced patch so don't know if it would be >>> considered small or not. >>> >>> Cheers, >>> David >>> If you would like your patch to be considered - please sign the OCA[2], that will be reflected at [3]. The process of becoming an OpenJDK contributor is described at [4]. Thanks, Ivan [1] http://openjdk.java.net/bylaws [2] http://www.oracle.com/technetwork/oca-405177.pdf [3] http://openjdk.java.net/census [4] http://openjdk.java.net/contribute/ On 06/09/2015 09:58, Sebastian Sickelmann wrote: > Please find my small patch[1] to javadoc in java.net.URL that adresses > JDK-4906983(javadoc-fix)[2]. > > -- Sebastian > > [1] > https://dl.dropboxusercontent.com/u/43692695/oss-patches/openjdk/JDK-4906983/index.html > > [2] https://bugs.openjdk.java.net/browse/JDK-4906983 >
Re: RFR 8080402: File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java
On 7 Sep 2015, at 17:41, Mark Sheppard wrote: > a couple of other considerations in the context of this issue perhaps? > > in this s is being duped onto fd, and part of the dup2 operation is the > closing of fd, but > what's is the expected state of file descriptor fd in the event of a dup2 > failure? I’m not sure that the documentation for dup2 gives us enough detail here. The only situation I can see where fd would not be closed is when EBADF is returned. Should not be an issue here. > s is closed in any case, but what about fd, should it be attended to if dup2 > < 0 > and closed ? is it still considered a valid fd? > > what can be said about the state of the encapsulating FileDescriptor > associated with fd? > at this stage can it still be considered valid? > should valid() return false? Probably, but may not be worth bothering with unless there are operations that can access it after this method throws. -Chris. > regards > Mark > > On 07/09/2015 14:29, Ivan Gerasimov wrote: >> Thanks! >> >> It looks good to me now. >> >> Sincerely yours, >> Ivan >> >> On 07.09.2015 16:08, Vyom Tewari wrote: >>> Hi All, >>> >>> Please find the latest diff, which contains the latest fix. >>> http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.02/ >>> >>> Thanks, >>> Vyom >>> >>> >>> On 9/7/2015 3:48 PM, Alan Bateman wrote: On 07/09/2015 10:26, Vyom Tewari wrote: > Hi everyone, > Can you please review my changes for below bug. > > Bug: > JDK-8080402 : File Leak in > jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java > Webrev: > http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.01/ > > This change ensure that if close() fails we throw correct io exception > and there is no file leak. close isn't restartable so I think we need to look at that while we are here. -Alan. >>> >>> >> >
Re: WebSocket client API
On 03/09/15 09:28, Andrej Golovnin wrote:
Hi Michael,
Can you explain why you need j.n.Proxy rather than the
String/InetSocketAddress
combination proposed for HttpClient?
String is not type safe. And allowing to define proxy only for the
specific protocol is not enough from my experience. Sometimes you have
to choose proxy based on the host of the URL you try to connect to.
What HttpClient.Builder.proxy() should really take as parameter, is
the j.n.ProxySelector. The most code, I have seen so far, does not
really care about proxies. But everyone expects that it still works
with proxies. j.n.URL.openConnection() gives us this magic because at
some point in sun.net.www.protocol.http.HttpURLConnection the method
ProxySelector.getDefault().select(URI) is called for the URL and the
connection is established using the proxy. The same behaviour I expect
from HttpClient and WebSocket. Therefore I think you have to deal with
j.n.Proxy and j.n.ProxySelector in the HttpClient code anyway. And
that's why I think you should use them in the Public API of HttpClient
and WebSocket as well.
Here is a small summary of the API I would like to see:
/**
* Defines a selector to be used by this client to select the proxy
server for a request.
* If no selector is defined for this client, the client will use
{@link j.n.ProxySelector.getDefault()} as it's selector.
*
* @param selectorthe selector to be used by this client; may not be null.
*
* @throws IllegalArgumentException if selector is null.
*/
j.n.HttpClient.Builder.proxySelector(j.n.ProxySelector selector)
/**
* Defines the proxy to be used by this request.
* If no proxy is defined for this request, the selector from the
HttpClient will be used to select the proxy.
*
* @param proxy the proxy to be used by this request; may not be null.
*
* @throws IllegalArgumentException if proxy is null.
*/
j.n.HttpRequest.Builder.proxy(j.n.Proxy proxy)
/**
* Defines the proxy to be used by this WebSocket.
* If no proxy is defined for this WebSocket, the default selector
{@link j.n.ProxySelector.getDefault()} will be used to select the
proxy.
*
* @param proxy the proxy to be used by this WebSocket; may not be null.
*
* @throws IllegalArgumentException if proxy is null.
*/
j.n.WebSocket.Builder.proxy(j.n.Proxy proxy)
As always fell free to use it or to ignore it. My code is already in
production and works. :-)
I hope this helps.
Best regards,
Andrej Golovnin
I was reluctant initially to use ProxySelector because of its complexity,
and the fact that SOCKS won't be supported,
but I think hooking into the default proxy selection mechanism is useful
(especially where system proxies are just expected to work)
I'll add this to the list of changes we'll consider after the initial
integration
but before JDK 9 is released.
Thanks,
Michael
Re: RFR 8080402: File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java
that's true, the documentation is a bit nebulous on this issue. but the inference is that the file descriptors are indeterminate state. some older man pages allude to this as per solaris man pages, close will " If close() is interrupted by a signal that is to be caught, it will return -1 with errno set to EINTR and the state of fildes is unspecified" if dup2(s, fd) is viewed as a combination of close(fd) and fcntl (s, F_DUP2FD, fd), and close is not restartable then similar semantics could be inferred for dup2 in a EINTR scenario? presume that subsequent call in the RESTARTABLE loop will return another error. On 08/09/2015 09:28, Chris Hegarty wrote: On 7 Sep 2015, at 17:41, Mark Sheppard wrote: a couple of other considerations in the context of this issue perhaps? in this s is being duped onto fd, and part of the dup2 operation is the closing of fd, but what's is the expected state of file descriptor fd in the event of a dup2 failure? I’m not sure that the documentation for dup2 gives us enough detail here. The only situation I can see where fd would not be closed is when EBADF is returned. Should not be an issue here. s is closed in any case, but what about fd, should it be attended to if dup2 < 0 and closed ? is it still considered a valid fd? what can be said about the state of the encapsulating FileDescriptor associated with fd? at this stage can it still be considered valid? should valid() return false? Probably, but may not be worth bothering with unless there are operations that can access it after this method throws. -Chris. regards Mark On 07/09/2015 14:29, Ivan Gerasimov wrote: Thanks! It looks good to me now. Sincerely yours, Ivan On 07.09.2015 16:08, Vyom Tewari wrote: Hi All, Please find the latest diff, which contains the latest fix. http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.02/ Thanks, Vyom On 9/7/2015 3:48 PM, Alan Bateman wrote: On 07/09/2015 10:26, Vyom Tewari wrote: Hi everyone, Can you please review my changes for below bug. Bug: JDK-8080402 : File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java Webrev: http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.01/ This change ensure that if close() fails we throw correct io exception and there is no file leak. close isn't restartable so I think we need to look at that while we are here. -Alan.
Re: WebSocket client API
Hi,
On Tue, Sep 1, 2015 at 10:45 PM, Pavel Rappo wrote:
> That's a very good question. The only way sending could be "back pressured" in
> the current API is by blocking completion of the returned
> CompletableFuture. But given the intention to provide a fully
> asynchronous
> non-blocking API, that's not a right thing to do. I have to think about a
> callback a user can install to be notified when they should put sending on
> hold:
>
> public interface BackpressureHandler {
>
> /**
>* Called by {@link WebSocket} no notify a client sending more data is
>* currently undesirable.
>*/
> void suspendSending();
>
> /**
>* Called by {@link WebSocket} no notify a client sending may be resumed.
>*/
> void resumeSending();
> }
Uh, please no.
We have already (partially) discussed this argument for the new
HttpClient APIs, and there is no need to reinvent the wheel every
time.
JDK 9 will like ship juc.Flow, so I would recommend to base on that
API rather than reinventing a new API every time to do backpressure.
>> WebSocket.Outgoing
>>
>> - What does binary(Iterable chunks)
>> and text(Iterable chunks) do?
>
> They provide a way for an API user to construct a WebSocket message (m) from a
> sequence (c[i], i: 0..n) of consecutive chunks, such that
>
> m = c[0] + c[1] + c[2] + ... c[n-1] + c[n]
>
> where "+" is concatenation operation. It's basically designed for cases when:
>
> 1. the overall size of the message is not known beforehand
> 2. the overall size of the message is too big to fit in a single
> ByteBuffer/CharSequence, or memory is a particularly scarce resource
> 3. it's more convenient for the user to provide something as a sequence of its
> parts rather than as a whole thing
>
> Think of it as some sort of data supplier. This thing also allows not to bring
> "fragmentation" concept in the API.
I don't see how this would work in general.
An application that acts as a proxy may have chunk1 available, know
that is not the last, but not have chunk2 available yet. And it cannot
buffer them.
An application cannot implement this with an Iterator.
Again, the problem is already solved by juc.Flow.
I don't understand why Outgoing has no instance methods ?
How is the implementation supposed to interact with it, and an
application write a WebSocket message if it cannot use the predefined
static methods ?
>> - Why is there no WebSocket.Outgoing chunking equivalent?
>> This will be important for those wanting to deal with streaming behaviors
>> over websocket.
>
> See the answer on the purpose of Outgoing above.
Nope, it's not covered.
Also, you need to address proper backpressure in the websocket proxy
case: read from downstream websocket and write to upstream websocket.
You cannot read more until the write has completed. This is currently
not doable with the API.
Again, look at how it is done in juc.Flow.
Where is TLS support ? Let's not make the same mistake already done in
javax.websocket :)
Thanks !
--
Simone Bordet
http://bordet.blogspot.com
---
Finally, no matter how good the architecture and design are,
to deliver bug-free software with optimal performance and reliability,
the implementation technique must be flawless. Victoria Livschitz
Re: WebSocket client API
On 08/09/15 17:29, Simone Bordet wrote: Where is TLS support ? Let's not make the same mistake already done in javax.websocket :) Thanks ! I notice it doesn't say so explicitly but "wss" urls are supported, and the relevant configuration comes from the HttpClient that the WebSocket belongs to. We probably should document this and maybe show an example or two. Michael
Re: WebSocket client API
> On 8 Sep 2015, at 18:27, Michael McMahon wrote: > > On 08/09/15 17:29, Simone Bordet wrote: >> Where is TLS support ? Let's not make the same mistake already done in >> javax.websocket :) Thanks ! > > I notice it doesn't say so explicitly but "wss" urls are supported, and the > relevant configuration > comes from the HttpClient that the WebSocket belongs to. > > We probably should document this and maybe show an example or two. > > Michael OK, will do it. Thanks for noticing!
Re: WebSocket client API
Hi Simone, thanks for looking at this! > On 8 Sep 2015, at 17:29, Simone Bordet wrote: > > Uh, please no. > > JDK 9 will like ship juc.Flow, so I would recommend to base on that > API rather than reinventing a new API every time to do backpressure. 1. To be honest, I don't think Flow API is a good match here. It's a high-level (very abstract) API. I assume people might later adapt WebSocket API to Flow API, but I still don't think we should implement it in terms of Flow API in the first place. For example, how do you see the Flow.Subscription.request(long n) would work here exactly? Should n (number of items) be a number of bytes? Frames? Chunks? Messages? Only bytes from the list above gives one control over back-pressure. Messages/Chunks/Frames may vary in size significantly. What would an app do if it asks for one more message, but the message would turn out to be a 100M file? 2. I wouldn't base the WebSocket client on an API that is not a part of JDK yet. A question if I may. Are there any examples of inherent issues in the model of communicating back-pressure proposed for the WebSocket API that are absent in Flow's approach? > I don't see how this would work in general. > > An application that acts as a proxy may have chunk1 available, know > that is not the last, but not have chunk2 available yet. And it cannot > buffer them. It just sends out the chunk1 to the next peer in the chain. How's that different from handling WebSocket frames on an intermediary? Think of chunks as frames if that helps. > An application cannot implement this with an Iterator. > Again, the problem is already solved by juc.Flow. Again, what problem? > I don't understand why Outgoing has no instance methods ? What instance methods would you like to see there? > How is the implementation supposed to interact with it, Have a look at [1] lines 702, 713, 724 and 735. The implementation will simply cast Outgoing to an appropriate subclass later. BTW, due to some previous criticism of Outgoing it will likely be removed. > and an > application write a WebSocket message if it cannot use the predefined > static methods ? Sorry, I didn't understand the question. >>> - Why is there no WebSocket.Outgoing chunking equivalent? >>> This will be important for those wanting to deal with streaming behaviors >>> over websocket. >> >> See the answer on the purpose of Outgoing above. > > Nope, it's not covered. > > Also, you need to address proper backpressure in the websocket proxy > case: read from downstream websocket and write to upstream websocket. > You cannot read more until the write has completed. This is currently > not doable with the API. I think we would use suspend/resume. The "proxy case" as far as I understand is no different from any other case where back-pressure control is needed. > Again, look at how it is done in juc.Flow. Could you please be more specific here? Are you talking about examples in java.util.concurrent.SubmissionPublisher? If yes then why you think this is more suitable for us? Thanks. -Pavel
RFR: JDK-4906983
Hi, Please find my small patch[1] to javadoc in java.net.URL that adresses JDK-4906983(javadoc-fix)[2]. I signed the SCA/OCA some time ago. Feel free to check at the OCA Signatures-List[3] thanks to david buck for hosting this patch on cr.openjdk.java.net. -- Sebastian Sickelmann [1] http://cr.openjdk.java.net/~dbuck/4906983.0/ [2] https://bugs.openjdk.java.net/browse/JDK-4906983 [3] http://www.oracle.com/technetwork/community/oca-486395.html
Re: WebSocket client API
Hi, On Tue, Sep 8, 2015 at 8:34 PM, Pavel Rappo wrote: > Hi Simone, thanks for looking at this! > >> On 8 Sep 2015, at 17:29, Simone Bordet wrote: >> >> Uh, please no. >> >> JDK 9 will like ship juc.Flow, so I would recommend to base on that >> API rather than reinventing a new API every time to do backpressure. > > 1. To be honest, I don't think Flow API is a good match here. It's a > high-level > (very abstract) API. You are misunderstanding it. It's actually a very low-level API, and some even call it a protocol. > I assume people might later adapt WebSocket API to Flow > API, but I still don't think we should implement it in terms of Flow API in > the > first place. For example, how do you see the > Flow.Subscription.request(lonJust rely on APIsg n) > would work here exactly? Should n (number of items) be a number of bytes? > Frames? Chunks? Messages? Only bytes from the list above gives one control > over > back-pressure. Messages/Chunks/Frames may vary in size significantly. Typically, request(long) should match what is being received in Subscriber.onNext(T). However, I know of people that want to be more fine grained, and may receive a ByteBuffer, but backpressure on the number of bytes consumed, but this requires some sort of buffering. It's an implementation detail; application typically use higher level APIs with a richer interface. > What would an app do if it asks for one more message, but the message would > turn > out to be a 100M file? If you ask for 1 message and you receive a 100M buffer, it is either already in memory of the publisher, or it is mapped to native memory. Either case, you can handle it just fine; when you are done with it, you can request one more of those buffers. > 2. I wouldn't base the WebSocket client on an API that is not a part of JDK > yet. Flow is scheduled to be part of JDK 9, AFAIK. If it's not anymore, then I would suggest at what Flow (or reactive streams) provides and do something similar. For your interest, we are talking about using Flow for the Servlet APIs and the new HttpClient. In any case, the basic use cases should work out of the box. With the current APIs, you cannot even write an echo service that is fully async. > A question if I may. Are there any examples of inherent issues in the model of > communicating back-pressure proposed for the WebSocket API that are absent in > Flow's approach? There are no examples of how one is supposed to use the API you are proposing. If I missed them, please be gentle and relink them to me. The solution you proposed with BackpressureHandler is racy and forces applications to do horrible synchronizations, or the implementation to impose a thread model, which is also a bad thing. I understand it's a proposal, but don't go that way. There exist proven solution already, just reuse them. Please, write a proxy example and show to all of us how the current proposed API would work. It must be fully async: you cannot read until you finished write and viceversa. >> I don't see how this would work in general. >>Because you mentioned it, >> An application that acts as a proxy may have chunk1 available, know >> that is not the last, but not have chunk2 available yet. And it cannot >> buffer them. > > It just sends out the chunk1 to the next peer in the chain. How's that > different > from handling WebSocket frames on an intermediary? Think of chunks as frames > if > that helps. Please, write the simple proxy I talk about. You will realize that you cannot do what you think you can. >> An application cannot implement this with an Iterator. >> Again, the problem is already solved by juc.Flow. > > Again, what problem? The async problem. The current API will force a thread to block. > BTW, due to some previous criticism of Outgoing it will likely be removed. Okay. > I think we would use suspend/resume. The "proxy case" as far as I understand > is > no different from any other case where back-pressure control is needed. Write the proxy case with the current API and post it somewhere for reference. I could not write it, but perhaps I have misunderstood the API proposal. I hope that in the spirit of other APIs, this API could be re-implemented using service providers (j.u.ServiceLoader). As such you cannot expect to create Oracle-private subclasses of Outgoing (or whatever solution replaces it) and have an alternative implementation cast them down to figure out how to extract the data they contains. Thanks ! -- Simone Bordet http://bordet.blogspot.com --- Finally, no matter how good the architecture and design are, to deliver bug-free software with optimal performance and reliability, the implementation technique must be flawless. Victoria Livschitz
