Hi Pavel,
On 01/06/2017 12:32, Pavel Rappo wrote:
Hi Daniel,
Thanks a lot for looking into this! I agree there's an issue exactly where you
pointed at. I'm sure it would be a good idea to write a test that covers this.
Unfortunately it would be quite a challenging task.
I agree it would be good to have a test :-(
These kind of complex state logic have a tendency to
bite you if you only test them by code reading.
I tried to rewrite the state machine the way that both fixes this bug and makes
the code easier to follow (I'm biased here obviously).
Can we throw an InternalError or assert if
124 reader.readFrame(data, frameConsumer);
does not advance the buffer position?
131 break;
hmmm... what if demand.get() is no longer 0 when we reach this line?
what if the cooperative handler already thinks that the current thread
will read the data? Is that possible?
best regards,
-- daniel
Please have a look at the updated webrev and tell me if you're generally happy
with this approach:
http://cr.openjdk.java.net/~prappo/8180155/webrev.01/
So here are a couple of points to keep in mind while reasoning about the code:
1. There's at most one channel readiness event registered at any given time
2. There's at most one thread executing `pushContinuously` at any given time
(enforced by CooperativeHandler)
Thanks!
On 31 May 2017, at 12:13, Daniel Fuchs <daniel.fu...@oracle.com> wrote:
Hi Pavel,
Receiver.java:
120 private void pushContinuously() {
121 while (true) {
122 if (!readable.get()) {
123 if (!initialized) {
124 initialized = true;
125 try {
126 channel.registerEvent(event);
127 } catch (IOException e) {
128 messageConsumer.onError(e);
129 }
130 }
131 break;
132 } else if (demand.get() > 0 && !handler.isStopped()) {
133 pushOnce();
134 } else {
135 break;
136 }
137 }
138 }
I think you might have an issue here, if the initialBuffer
already contains all the data that there is to be read, then
the readable.get() will be false, initialized will be false,
pushOnce() will not be called, and you're going to register
an event that will never be triggered.
best regards,
-- daniel
On 31/05/2017 11:30, Pavel Rappo wrote:
Hello,
Please review the following change:
http://cr.openjdk.java.net/~prappo/8180155/webrev.00/
This change addresses 2 separate issues:
https://bugs.openjdk.java.net/browse/JDK-8180155
https://bugs.openjdk.java.net/browse/JDK-8156518
The first one is a busy-wait for data on the socket which manifests itself as
non-returning invocation of WebSocket.request(long).
The second one is an issue with a timeout for an asynchronous HTTP request.
This issue affects both HttpClient and WebSocket implementations.
Thanks,
-Pavel
