You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by re...@apache.org on 2017/12/01 16:54:38 UTC
svn commit: r1816887 - in /tomcat/trunk:
java/org/apache/coyote/http11/upgrade/UpgradeServletInputStream.java
webapps/docs/changelog.xml
Author: remm
Date: Fri Dec 1 16:54:38 2017
New Revision: 1816887
URL: http://svn.apache.org/viewvc?rev=1816887&view=rev
Log:
Do not call onDataAvailable without data to read. I tried method with less side effects, but they all caused problems elsewhere, while this one passes the testsuite and everything else I could test it with.
Modified:
tomcat/trunk/java/org/apache/coyote/http11/upgrade/UpgradeServletInputStream.java
tomcat/trunk/webapps/docs/changelog.xml
Modified: tomcat/trunk/java/org/apache/coyote/http11/upgrade/UpgradeServletInputStream.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http11/upgrade/UpgradeServletInputStream.java?rev=1816887&r1=1816886&r2=1816887&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/http11/upgrade/UpgradeServletInputStream.java (original)
+++ tomcat/trunk/java/org/apache/coyote/http11/upgrade/UpgradeServletInputStream.java Fri Dec 1 16:54:38 2017
@@ -203,8 +203,12 @@ public class UpgradeServletInputStream e
final void onDataAvailable() {
- if (listener == null) {
- return;
+ try {
+ if (listener == null || !socketWrapper.isReadyForRead()) {
+ return;
+ }
+ } catch (IOException e) {
+ onError(e);
}
ready = Boolean.TRUE;
ClassLoader oldCL = processor.getUpgradeToken().getContextBind().bind(false, null);
Modified: tomcat/trunk/webapps/docs/changelog.xml
URL: http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/changelog.xml?rev=1816887&r1=1816886&r2=1816887&view=diff
==============================================================================
--- tomcat/trunk/webapps/docs/changelog.xml (original)
+++ tomcat/trunk/webapps/docs/changelog.xml Fri Dec 1 16:54:38 2017
@@ -55,12 +55,15 @@
</fix>
</changelog>
</subsection>
- <subsection>
+ <subsection name="Coyote">
<changelog>
<add>
<bug>60276</bug>: Implement GZIP compression support for responses
served over HTTP/2. (markt)
</add>
+ <fix>
+ Do not call onDataAvailable without any data to read. (remm)
+ </fix>
</changelog>
</subsection>
<subsection name="Other">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1816887 - in /tomcat/trunk: java/org/apache/coyote/http11/upgrade/UpgradeServletInputStream.java
webapps/docs/changelog.xml
Posted by Rémy Maucherat <re...@apache.org>.
On Sat, Dec 16, 2017 at 6:13 PM, Mark Thomas <ma...@apache.org> wrote:
> On 15/12/2017 15:15, Rémy Maucherat wrote:
> > On Fri, Dec 15, 2017 at 12:33 PM, Mark Thomas <ma...@apache.org> wrote:
> >
> >> On 01/12/2017 16:54, remm@apache.org wrote:
> >>> Author: remm
> >>> Date: Fri Dec 1 16:54:38 2017
> >>> New Revision: 1816887
> >>>
> >>> URL: http://svn.apache.org/viewvc?rev=1816887&view=rev
> >>> Log:
> >>> Do not call onDataAvailable without data to read. I tried method with
> >> less side effects, but they all caused problems elsewhere, while this
> one
> >> passes the testsuite and everything else I could test it with.
> >>
> >> This commit is what triggered the failure in TestUpgrade that Gump is
> >> current complaining about for APR/native.
> >>
> >> I can reproduce this failure on OSX. I haven't tried on other platforms.
> >>
> >> The patch looks right to me. I'm wondering if this change has simply
> >> exposed an issue somewhere in the APR/native connector. I'll take a
> look.
> >>
> > Oops, sorry. I didn't see TestUpgrade failing on CI, so that was it for
> > me, I usually don't test APR anymore.
> >
> > Since it fails for me too, I debugged it, and the server IO events are
> the
> > same for APR (3 onDataAvailable calls), with the data written being the
> > same as well (2*16 bytes in onWritePossible). I'm not sure yet why the
> > client fails to read it with the patch in place.
>
> Figured it out.
>
> EOF isn't handled correctly (likely my fault) so onAllDataRead() isn't
> triggered and no data is sent to the client. The lack of data triggers
> the test failure.
>
> Fix to follow shortly.
>
> I still need to investigate but I suspect some back-ports will be
> required.
>
> Thanks for the fix, hopefully it'll be fine now.
Rémy
Re: svn commit: r1816887 - in /tomcat/trunk:
java/org/apache/coyote/http11/upgrade/UpgradeServletInputStream.java
webapps/docs/changelog.xml
Posted by Mark Thomas <ma...@apache.org>.
On 15/12/2017 15:15, Rémy Maucherat wrote:
> On Fri, Dec 15, 2017 at 12:33 PM, Mark Thomas <ma...@apache.org> wrote:
>
>> On 01/12/2017 16:54, remm@apache.org wrote:
>>> Author: remm
>>> Date: Fri Dec 1 16:54:38 2017
>>> New Revision: 1816887
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1816887&view=rev
>>> Log:
>>> Do not call onDataAvailable without data to read. I tried method with
>> less side effects, but they all caused problems elsewhere, while this one
>> passes the testsuite and everything else I could test it with.
>>
>> This commit is what triggered the failure in TestUpgrade that Gump is
>> current complaining about for APR/native.
>>
>> I can reproduce this failure on OSX. I haven't tried on other platforms.
>>
>> The patch looks right to me. I'm wondering if this change has simply
>> exposed an issue somewhere in the APR/native connector. I'll take a look.
>>
> Oops, sorry. I didn't see TestUpgrade failing on CI, so that was it for
> me, I usually don't test APR anymore.
>
> Since it fails for me too, I debugged it, and the server IO events are the
> same for APR (3 onDataAvailable calls), with the data written being the
> same as well (2*16 bytes in onWritePossible). I'm not sure yet why the
> client fails to read it with the patch in place.
Figured it out.
EOF isn't handled correctly (likely my fault) so onAllDataRead() isn't
triggered and no data is sent to the client. The lack of data triggers
the test failure.
Fix to follow shortly.
I still need to investigate but I suspect some back-ports will be required.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1816887 - in /tomcat/trunk: java/org/apache/coyote/http11/upgrade/UpgradeServletInputStream.java
webapps/docs/changelog.xml
Posted by Rémy Maucherat <re...@apache.org>.
On Fri, Dec 15, 2017 at 12:33 PM, Mark Thomas <ma...@apache.org> wrote:
> On 01/12/2017 16:54, remm@apache.org wrote:
> > Author: remm
> > Date: Fri Dec 1 16:54:38 2017
> > New Revision: 1816887
> >
> > URL: http://svn.apache.org/viewvc?rev=1816887&view=rev
> > Log:
> > Do not call onDataAvailable without data to read. I tried method with
> less side effects, but they all caused problems elsewhere, while this one
> passes the testsuite and everything else I could test it with.
>
> This commit is what triggered the failure in TestUpgrade that Gump is
> current complaining about for APR/native.
>
> I can reproduce this failure on OSX. I haven't tried on other platforms.
>
> The patch looks right to me. I'm wondering if this change has simply
> exposed an issue somewhere in the APR/native connector. I'll take a look.
>
> Oops, sorry. I didn't see TestUpgrade failing on CI, so that was it for
me, I usually don't test APR anymore.
Since it fails for me too, I debugged it, and the server IO events are the
same for APR (3 onDataAvailable calls), with the data written being the
same as well (2*16 bytes in onWritePossible). I'm not sure yet why the
client fails to read it with the patch in place.
Rémy
Re: svn commit: r1816887 - in /tomcat/trunk:
java/org/apache/coyote/http11/upgrade/UpgradeServletInputStream.java
webapps/docs/changelog.xml
Posted by Mark Thomas <ma...@apache.org>.
On 01/12/2017 16:54, remm@apache.org wrote:
> Author: remm
> Date: Fri Dec 1 16:54:38 2017
> New Revision: 1816887
>
> URL: http://svn.apache.org/viewvc?rev=1816887&view=rev
> Log:
> Do not call onDataAvailable without data to read. I tried method with less side effects, but they all caused problems elsewhere, while this one passes the testsuite and everything else I could test it with.
This commit is what triggered the failure in TestUpgrade that Gump is
current complaining about for APR/native.
I can reproduce this failure on OSX. I haven't tried on other platforms.
The patch looks right to me. I'm wondering if this change has simply
exposed an issue somewhere in the APR/native connector. I'll take a look.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org