Re: Proposed change to STREAMS_DROP_EMPTY_MESSAGES system property
I went for c in the end. The TCK passes as do all of our own tests.
Back-porting the change is still on my personal TODO list.
Mark
On 05/02/2019 22:02, Romain Manni-Bucau wrote:
> As an user c sounds ok. Worse case maybe keep it for 1 release with some
> comm to have time to migrate if relevant.
>
> Le mar. 5 févr. 2019 à 20:10, Rémy Maucherat a écrit :
>
>> On Tue, Feb 5, 2019 at 7:58 PM Mark Thomas wrote:
>>
>>> Hi,
>>>
>>> org.apache.tomcat.websocket.STREAMS_DROP_EMPTY_MESSAGES
>>>
>>> The above system property was added to address an issue with Tomcat's
>>> WebSocket implementation not passing the TCK. Tomcat was sending empty
>>> messages when the TCK wasn't expecting a message to be sent.
>>>
>>> Now that we have access to the TCK I have been able to track down the
>>> root cause of these failures.
>>>
>>> The root cause is this code in WsRemoteEndpointImplBase:
>>> ...
>>> } else if (encoder instanceof Encoder.TextStream) {
>>> try (Writer w = getSendWriter()) {
>>> ((Encoder.TextStream) encoder).encode(obj, w);
>>> }
>>> }
>>>
>>> The call to getSendWriter() triggers the state change that a write has
>>> started. Then the exception is thrown and because of the
>>> try-with-resources close() is called. That triggers the end of message
>>> state change hence a zero length message is written.
>>>
>>> The STREAMS_DROP_EMPTY_MESSAGES causes all empty messages (not just in
>>> this error case) to be dropped.
>>>
>>> I'm currently thinking that handling of this error case and
>>> STREAMS_DROP_EMPTY_MESSAGES should be decoupled. The idea is that, in
>>> the above case, obtaining the Writer is delayed until the encoder tries
>>> to write bytes.
>>>
>>> If the above change is implemented then what should be happen to
>>> STREAMS_DROP_EMPTY_MESSAGES?
>>> a) keep it as is
>>> b) deprecate it and remove it in 10.x
>>> c) remove it now
>>>
>>> I'm leaning towards b)
>>>
>>
>> You could probably do c) as the only purpose was to stick to the TCK
>> behavior. When there is no spec, the TCK is supposed to be the referee (and
>> I don't really see why empty messages are useful).
>>
>> Rémy
>>
>>
>>>
>>> Thoughts?
>>>
>>> Mark
>>>
>>> -
>>> To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
>>> For additional commands, e-mail: dev-h...@tomcat.apache.org
>>>
>>>
>>
>
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: Proposed change to STREAMS_DROP_EMPTY_MESSAGES system property
As an user c sounds ok. Worse case maybe keep it for 1 release with some
comm to have time to migrate if relevant.
Le mar. 5 févr. 2019 à 20:10, Rémy Maucherat a écrit :
> On Tue, Feb 5, 2019 at 7:58 PM Mark Thomas wrote:
>
> > Hi,
> >
> > org.apache.tomcat.websocket.STREAMS_DROP_EMPTY_MESSAGES
> >
> > The above system property was added to address an issue with Tomcat's
> > WebSocket implementation not passing the TCK. Tomcat was sending empty
> > messages when the TCK wasn't expecting a message to be sent.
> >
> > Now that we have access to the TCK I have been able to track down the
> > root cause of these failures.
> >
> > The root cause is this code in WsRemoteEndpointImplBase:
> > ...
> > } else if (encoder instanceof Encoder.TextStream) {
> > try (Writer w = getSendWriter()) {
> > ((Encoder.TextStream) encoder).encode(obj, w);
> > }
> > }
> >
> > The call to getSendWriter() triggers the state change that a write has
> > started. Then the exception is thrown and because of the
> > try-with-resources close() is called. That triggers the end of message
> > state change hence a zero length message is written.
> >
> > The STREAMS_DROP_EMPTY_MESSAGES causes all empty messages (not just in
> > this error case) to be dropped.
> >
> > I'm currently thinking that handling of this error case and
> > STREAMS_DROP_EMPTY_MESSAGES should be decoupled. The idea is that, in
> > the above case, obtaining the Writer is delayed until the encoder tries
> > to write bytes.
> >
> > If the above change is implemented then what should be happen to
> > STREAMS_DROP_EMPTY_MESSAGES?
> > a) keep it as is
> > b) deprecate it and remove it in 10.x
> > c) remove it now
> >
> > I'm leaning towards b)
> >
>
> You could probably do c) as the only purpose was to stick to the TCK
> behavior. When there is no spec, the TCK is supposed to be the referee (and
> I don't really see why empty messages are useful).
>
> Rémy
>
>
> >
> > Thoughts?
> >
> > Mark
> >
> > -
> > To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
> > For additional commands, e-mail: dev-h...@tomcat.apache.org
> >
> >
>
Re: Proposed change to STREAMS_DROP_EMPTY_MESSAGES system property
On Tue, Feb 5, 2019 at 7:58 PM Mark Thomas wrote:
> Hi,
>
> org.apache.tomcat.websocket.STREAMS_DROP_EMPTY_MESSAGES
>
> The above system property was added to address an issue with Tomcat's
> WebSocket implementation not passing the TCK. Tomcat was sending empty
> messages when the TCK wasn't expecting a message to be sent.
>
> Now that we have access to the TCK I have been able to track down the
> root cause of these failures.
>
> The root cause is this code in WsRemoteEndpointImplBase:
> ...
> } else if (encoder instanceof Encoder.TextStream) {
> try (Writer w = getSendWriter()) {
> ((Encoder.TextStream) encoder).encode(obj, w);
> }
> }
>
> The call to getSendWriter() triggers the state change that a write has
> started. Then the exception is thrown and because of the
> try-with-resources close() is called. That triggers the end of message
> state change hence a zero length message is written.
>
> The STREAMS_DROP_EMPTY_MESSAGES causes all empty messages (not just in
> this error case) to be dropped.
>
> I'm currently thinking that handling of this error case and
> STREAMS_DROP_EMPTY_MESSAGES should be decoupled. The idea is that, in
> the above case, obtaining the Writer is delayed until the encoder tries
> to write bytes.
>
> If the above change is implemented then what should be happen to
> STREAMS_DROP_EMPTY_MESSAGES?
> a) keep it as is
> b) deprecate it and remove it in 10.x
> c) remove it now
>
> I'm leaning towards b)
>
You could probably do c) as the only purpose was to stick to the TCK
behavior. When there is no spec, the TCK is supposed to be the referee (and
I don't really see why empty messages are useful).
Rémy
>
> Thoughts?
>
> Mark
>
> -
> To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
> For additional commands, e-mail: dev-h...@tomcat.apache.org
>
>
