Clojurians Log v2

Double checking that this is with Jetty, right?

Jetty, correct

I'm re-implementing it myself and the ByteBuffer seems to change when sending and receiving it over a channel, at least the lim gets set to 0.

That's where I was headed; something needs to "flip" the ByteBuffer so that Jetty is ready to read out of it what was originally written into it, if I remember this stuff correctly.

So I wonder if that's something Pedestal is not doing, that the native .sendBinary is doing?

(extend-protocol WebSocketSendAsync
  String
  (ws-send-async [msg ^RemoteEndpoint$Async remote-endpoint]
    (let [p-chan (async/promise-chan)]
      (.sendText remote-endpoint msg (send-handler p-chan))
      p-chan))

  ByteBuffer
  (ws-send-async [msg ^RemoteEndpoint$Async remote-endpoint]
    (let [p-chan (async/promise-chan)]
      (.sendBinary remote-endpoint msg (send-handler p-chan))
      p-chan)))

Both methods end up calling .sendBinary, the only difference seems to me is that there is a channel involved

Pedestal is using .getAsyncRemote, though.

I need to check the tests to see if they properly exercise this code.

Tests look good AFAIK, definitely exercising ByteBuffers. But small ones, from byte arrays, not HeapByteBuffer. Not sure that makes a difference.

If I try sending a duplicate via asReadOnlyBuffer it works 🤷

That's extra odd because asReadOnlyBuffer is a shallow copy, doesn't look like it changes position or limit.

The problem is that my tests to a .rewind after writing to the channel so they can compare what gets sent over the wire correctly. That may be functionality that's required is some way. Though that doesn't explain why things work for you hitting the APIs directly. it's not obvious what the difference is.

Does something happen to the buffer when the handler for the binary message is finished?

Rather, in this example, the socket is receving a binary message, and my code is forwarding it to another channel.

No, the BB is passed to .sendBinary and otherwise forgotten by Pedestal.

Maybe GC'd?

Should be.

So maybe your calling code is in a loop? And it is reading into the buffer, passing it to Pedestal, then back to the loop, clearing/flipping the buffer, reading more data? That could create a race condition where, by the time Pedestal sees the BB and by the time the async Jetty stuff uses the buffer, it cleared and so nothing gets written out. The asReadOnlyBuffer insulates you from changes to position/limit of original BB but not from changes to the data inside the BB.

If that's the case, it's a problem ... Pedestal could to the asReadOnlyBuffer, but that's only part of the problem.

Looking at your code ... same ByteBuffer message sent to different channels, I think that's a problem in the calling code.

Docs should be specific that "ByteBuffer must not be modified by caller after writing into channel" and a reminder that "reading from a BB is a destructive operation". If this fits that scenario.

My example has a bunch of unnecessary stuff happening, I'm able to reproduce the issue in this simpler one

(defn handle-ws-binary [uuid message]
  (let [data (deref sessions)
        chan (get-in data [:conns uuid :chan])]

    ;; Sends empty binary message
    #_(async/>!! chan message)

    ;; Sends full binary message
    (async/>!! chan (.asReadOnlyBuffer message))))

Ok, that's helpful.

Could you try using an async remote?

That's what the Pedestal code does.

My re-implemented start-ws-connection does this

(defn start-ws-conn [session chan]
  (async/go-loop []
    (if-let [message (and (.isOpen session) (async/<! chan))]
      (do (condp instance? message
            String     (.sendText   (.getAsyncRemote session) message)
            ByteBuffer (.sendBinary (.getAsyncRemote session) message))
          (recur))
      (.close session))))

Basically the same thing but with fewer moving parts so I could inspect what was happening

public Future sendBinary(ByteBuffer data) { What if you invoke .get on the returned java.util.concurrent.Future?

Umm returns nil as described and the binary message is still empty (sending the non-duplicated one)

Ok, so does your start-ws-conn work? Are we looking for the difference between yours and Pedestal's?

io.pedestal.websocket/start-ws-connection uses the extra argument because it is running in the core.async thread pool and has to avoid doing an I/O, so it is asking Jetty to do the work async, and using the promise to know when the send completes w/o blocking (the way a future blocks).

Both my start-ws-conn and Pedestal's start-ws-connection work when sending a duplicated ByteBuffer, neither work with the original buffer

Ok, that's useful to know.

And, again, the message is sent but empty.

Yep exactly

Well, the LLM gods did research and say "yes you must make a read only copy" https://www.perplexity.ai/search/jetty-11-pass-a-bytebuffer-to-7Wd4ehb4SqeXPq4asgEVvw I'm still checking the references ...

So I'd love to see the code that is writing into your version of start-ws-conn's chan.

Because it gets back to the "are you reusing the ByteBuffer before Jetty has a chance to pipe it out on the web socket?"

The LLM references so far don't back up its conclusions, I'm thinking hallucination.

Here is the whole server 😄

If it sounds like I'm dragging my feet on just adding .asReadOnlyBuffer, it's because that could potentially mask the problem without fixing it. The read-only BB will have its own mark/position/limit but will share the underlying byte array (for HeapByteBuffer) or stretch of memory (for direct) that is mutable.

That could cause much more damaging and hard to diagnose bugs later. For example, leaking data for one customer into a different customer.

No, I think it makes more sense to figure out exactly what is happening the original buffer before blindly duplicating it. And the work-around works for my use-case for now so I'm not in any rush

Thanks for all your help though! Really pleased with the changes to Pedestal so far 💯 let me know if theres any more information I can provide for this issue

I'm looking at your code, may take a little bit.

Issue with ByteBuffers is that they are mutable, and potentially very expensive to duplicate properly. If you start with a direct BB and make a heap BB copy of it, you've undone all the performance bonuses you get from a direct BB in the first place.

So ... in your code handle-ws-binary is passing the ByteBuffer messages it got from jetty websocket-land into the channel and back out to Jetty. But that's all async. So I'm going to peek at the code in Jetty that ultimately calls the handle-ws-binary function. Because I think that code may be looping, resetting the ByteBuffer etc., before the async code has a chance to kick in a short time later.

It would make sense if Jetty was doing something to the buffer after the handler returns. And why a direct synchronous call to .sendBinary would work

> Developers should not continue to reference message objects of type http://java.io.Reader, java.nio.ByteBuffer or java.io.InputStream after the completion of the onMessage() call, since they may be recycled by the implementation.

Should I prefer a synchronous call to .sendBinary here instead of using a duplicate?

No, I think what you want to do is make a proper, deep copy of the BB.

But I'll beef up the Pedestal docs to make this very clear.

Deep copy: ByteBuffer/allocate a new buffer, then (.put new-bb message).

Ok, then. The code was already in that state, but the docs were not, which I've now fixed.