Hello Stuart,
thanks for your input!
please see inline.
On 12/01/15 21:37, Stuart Douglas wrote:
>
>
> ---------- Forwarded message ---------
> From: Stuart Douglas <stuart.w.douglas_at_gmail.com
> <mailto:stuart.w.douglas_at_gmail.com>>
> Date: Tue Jan 13 2015 at 7:34:53 AM
> Subject: Re: [jsr356-users] [jsr356-experts] Re: Clarification required
> on sending messages concurrently
> To: <jsr356-experts_at_websocket-spec.java.net
> <mailto:jsr356-experts_at_websocket-spec.java.net>>
>
>
> On Tue Jan 13 2015 at 5:53:30 AM Pavel Bucek <pavel.bucek_at_oracle.com
> <mailto:pavel.bucek_at_oracle.com>> wrote:
>
> Hi Mark,
>
> seems like there might be some misinterpretation in the original thread
> - I read the reply from Danny as "container has to send the whole
> message before sending another one"; it does not seem to concern async
> messages, because container already has the message and it will be sent
> (or @OnError invoked).
>
> Your sample can be reduced to:
>
> session.getAsyncRemote().sendText(message);
> session.getBasicRemote().sendText(message);
>
>
> and I don't think this should end up in an exception. There must be some
> kind of queue implemented in the container and second call will wait for
> first message to be sent, but that's all. Consider Session class
> javadoc:
>
> * Session objects may be called by multiple threads. Implementations
> must
> * ensure the integrity of the mutable properties of the session under
> such circumstances.
>
>
> There is no API for users to get the state of current Session instance
> like "SENDING" or "READY_TO_SEND". When shared and used from multiple
> threads, it would not be easily possible to reliably send a message, if
> the sending state of individual messages could cause an exception.
>
>
> I think the problematic case here is code something like the following
> contrived example:
>
> for(int i = 0; i < 10000; ++i) {
> session.getAsyncRemote().sendText("Hello " + i);
> }
>
> If the connection is not fast enough to send all these the container
> will have to buffer them, and in some situations the container may end
> up having to buffer a lot of data (a more realistic case would simply be
> a slow connection, and the server starts generating messages faster than
> they can be sent to the client).
That is something which can be handled even now; or at least that is how
I read the paragraph 5.2.3 from the spec:
""
5.2.3 Errors Originating in the Container and/or Underlying Connection
A wide variety of runtime errors may occur during the functioning of an
endpoint. These may including broken underlying connections, occasional
communication errors handling incoming and outgoing messages, or fatal
errors communicating with a peer.
***Implementations or their administrators judging such errors to be
fatal to the correct functioning of the endpoint may close the endpoint
connection, making an attempt to informing both participants using the
onClose() method.***
Containers judging such errors to be non-fatal to the correct
functioning of the endpoint may allow the endpoint to continue
functioning, but must report the error in message processing either as a
checked exception returned by one of the send operations, or by
delivering a the SessionException to the endpoints error handling
method, if present, or by logging the error for later analysis.
[WSC-5.2.3-1]
""
It seems like implementation can choose what to do - and it can be more
complex scenario which will reflect some runtime connection properties
or just some simple error counter; if the condition is met,
implementation can decide that this connection/Session is no longer
viable and close it.
Applied to your usecase, I believe the container should return futures
with error result (isDone() returns true, get() throws an exception).
Also, the implementation can decide that the Session cannot be
recovered, and close it. Since Async RemoteEndpoint send method does not
throw an exception, implementation should invoke onError with
SessionException (last sentence in 5.2.3).
>
>
> The other thing is the original usecase. I believe we should signal
> something when user tries to send a message in the middle of sending
> another partial message. I will have to investigate little bit more
> about this, but I can say that the RI does not do this at the moment.
>
> Looking at this issue from broader perspective, it could be nice to have
> the alternative to this, something as "force send", since we might get
> to the state when we want to send some message, no matter what is
> happening in other threads. Just for the sake of completeness, here is
> the usecase:
>
> session.getBasicRemote().sendText(message, false); // partial
> session.getBasicRemote().sendText(message);
>
>
> (Currently, RI sends the second (whole) message as part of the previous
> one; or not really, it is just interpreted in that way..).
>
>
> Looking at our (Undertow's) implementation of this it looks like this
> example would block indefinitely, or at least until another thread calls
>
> session.getBasicRemote().sendText(message, true);
>
> In this case our underlying websocket implementation will delay sending
> the second message until the first is done, so as blocking IO is in use
> it would require another thread to finish sending the first message.
That is what I would expect should happen, but it is also dangerous -
the thread could be blocked for a long time.
> Regarding your 'Force Send' idea I am not sure how workable it is, as if
> any part of the first message has already hit the wire you have to wait
> for it to be completed. I guess if you have a few messages queued it
> could move the message to the front of the queue.
yeah, it was just "brainstorming idea" and most likely incorrect one.
Anyway, I still dislike the fact, that user cannot really know whether
the message will be sent "right away" or if there is a possibility to
have current thread blocked for a while. And I'm pretty sure we don't
want to force users to constructs like
try {
session.getBasicRemote().sendText(MSG);
} catch (IllegalStateException e) {
// .. handle error
}
>
> Following is even more evil:
>
> session.getBasicRemote().sendText(m, false); // partial text
>
> session.getBasicRemote().sendBinary(ByteBuffer.wrap(m.getBytes()));
>
> (this is interpreted in the same way as previous; if you think of the
> frame content and order, it makes sense that the other side interpreted
> it as one text message. Problem is that we might need to detect this on
> the sender side and throw an exception probably..)
>
>
> We currently throw an exception in this case, although there is no real
> reason why we could not just hold off sending the binary message till
> the first one is complete.
That seems little inconsistent, given the handling of the other case,
but it seems like it is still spec compliant.. we might want to make it
clearer in next version of the spec.
Thanks,
Pavel
>
> Stuart
>
>
>
> Thanks and regards,
> Pavel
>
>
> On 12/01/15 11:21, Mark Thomas wrote:
> > Looking back though the EG discussions, we discussed concurrency and
> > agreed that one message had to complete before the next was sent
> [1]. No
> > distinction was made between synchronous and asynchronous messages.
> >
> > The discussion in [1] happened before the split between
> > RemoteEndpoint.[Basic|Async] and after the split restriction
> described
> > in [1] only appears in the Javadoc for RemoteEndpoint.Basic.
> >
> > My question is this:
> >
> > Does the restriction agreed in [1] apply to async messages or not?
> >
> > I believe that the restriction does apply. Without it, there is no
> > mechanism for the container to signal to the application to stop
> sending
> > messages if the client isn't reading them fast enough.
> >
> > By way of example, consider the following:
> > @OnMessage
> > public String onMessage(String message, Session session) {
> > received.add(message);
> > session.getAsyncRemote().sendText(MESSAGE_ONE);
> > return message;
> > }
> >
> >
> > My expectation is that, unless the async message completes very
> quickly
> > (unlikely) then an ISE will be thrown because the async message
> is still
> > in progress when the sync message send is triggered by the return
> value.
> >
> > That said, if batching is enabled then it is much more likely
> that the
> > async message would complete instantly and that the sync message
> would
> > then be sent.
> >
> > For bonus points, assuming that the batch buffer is big enough
> for both
> > the async and sync message, is the use of the return value to
> send the
> > message a trigger to flush the batch? I can seen no reason why this
> > should be the case but the question popped up on a Tomcat list so I
> > wanted to check here.
> >
> > Mark
> >
> >
> > [1]
> >
> https://java.net/projects/websocket-spec/lists/jsr356-experts/archive/2013-02/message/48
> <https://java.net/projects/websocket-spec/lists/jsr356-experts/archive/2013-02/message/48>
> >
>