Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Binary data support #398

Closed
wants to merge 2 commits into from
Closed

Binary data support #398

wants to merge 2 commits into from

Conversation

rosejn
Copy link
Contributor

@rosejn rosejn commented Jan 21, 2022

With these modifications I was able to implement a msgpack packer in both Clojure and Clojurescript, so we are now transmitting large binary typed arrays over the websocket channel. I'm also working on unifying clojure-msgpack and msgpack-cljs libs, and once that work is done I'm happy to also provide the packers so future Sente users can have plug and play binary data support. Since often times people want to use websockets for dashboards which can often be data heavy, I think this will be a welcome addition. Please let me know if you have any thoughts or feedback, and I'm happy to work with you on these changes if you have other ideas. Thanks for Sente!

@ptaoussanis
Copy link
Member

@rosejn Hi Jeff, thanks a lot for this! Apologies for the long delay replying.

In principle very happy to add support for binary packers to Sente.
Unfortunately the current PR would break compatibility between new packers and old unpackers.

I'm not opposed to that if it's completely unavoidable - but it would be nice to try and preserve backwards-compatibility if possible.

To better understand your needs, it would be helpful to see an example of what your binary IPacker implementation actually looks like. Would it be possible to share some more details?

Aiming to cut a new Sente release in the next few days, so there's an opportunity to get this change included soon if I can get your feedback before then.

Thanks!

ptaoussanis pushed a commit that referenced this pull request May 31, 2022
This commit changes an implementation detail of the packed
message format [1] to enable extra flexibility for custom
IPacker implementations.
ptaoussanis pushed a commit that referenced this pull request May 31, 2022
This commit changes an implementation detail of the packed
message format [1] to enable extra flexibility for custom
IPacker implementations.

[1] Stop automatic "+"/"-" prefixing of packed output.

  This was originally done as a micro-optimization to keep packed
  output as short as possible for string packaged message formats.

  The optimization really doesn't make much difference, so isn't
  worth the loss in flexibility.
ptaoussanis pushed a commit that referenced this pull request May 31, 2022
This commit changes an implementation detail of the packed
message format [1] to enable extra flexibility for custom
IPacker implementations.

[1] Stop automatic "+"/"-" prefixing of packed output.

  This was originally done as a micro-optimization to keep packed
  output as short as possible for string packaged message formats.

  The optimization really doesn't make much difference, so isn't
  worth the loss in flexibility.
ptaoussanis pushed a commit that referenced this pull request May 31, 2022
This commit changes an implementation detail of the packed
message format [1] to enable extra flexibility for custom
IPacker implementations.

[1] Stop automatic "+"/"-" prefixing of packed output.

  This was originally done as a micro-optimization to keep packed
  output as short as possible for string packaged message formats.

  The optimization really doesn't make much difference, so isn't
  worth the loss in flexibility.
@rosejn
Copy link
Contributor Author

rosejn commented May 31, 2022

Hi, so I put together a cljc msgpack library that merged the old libs I had referenced. I contacted those authors too and nobody replied, so I'll probably just repackage this and publish it so others can use it.

https://github.com/rosejn/clojure-msgpack/tree/cljc

Here's the code for our current packer:

(ns mxv.server.stream.msgpack
  (:require
    [msgpack.core :as msgpack]
    [msgpack.extensions]
    [msgpack.macros :refer [extend-msgpack]]
    [taoensso.sente.interfaces :refer [IPacker]])
  (:import
    [java.nio ByteBuffer FloatBuffer ByteOrder]
    java.time.Instant))

(defn hex
  "Convert byte sequence to hex string"
  [coll]
  (let [hex [\0 \1 \2 \3 \4 \5 \6 \7 \8 \9 \a \b \c \d \e \f]]
      (letfn [(hexify-byte [b]
        (let [v (bit-and b 0xFF)]
          [(hex (bit-shift-right v 4)) (hex (bit-and v 0x0F))]))]
        (apply str (interleave
                     (mapcat hexify-byte coll)
                     (repeat " "))))))


; Add packing support for java.time.Instant
(extend-msgpack
  java.time.Instant
  100

  (pack
    [instant]
    (msgpack/pack (.toString instant)))

  (unpack
    [bytes]
    (java.time.Instant/parse (String. bytes))))


(deftype MsgPacker
  []
  IPacker
  (pack   [_ x]
    (msgpack/pack x))

  (unpack [_ s]
    (let [msg (msgpack/unpack s)]
      msg)))

(defn msgpack-packer
  []
  (MsgPacker.))

And on the client it's equally simple:

(deftype MsgPacker []
  IPacker
  (pack   [_ x] (msgpack/pack x))
  (unpack [_ s] (msgpack/unpack s)))


(defn msg-packer
  []
  (MsgPacker.))

Let me know if you need anything else. It would be great to get this support into mainline Sente so we don't need to reference my own branch in our production project anymore. Thanks!

ptaoussanis pushed a commit that referenced this pull request Jun 1, 2022
…anis)

This commit changes an implementation detail of the packed
message format [1] to enable non-string IPacker implementations.

See new `*write-legacy-pack-format?*` var for more info.

[1] Stop automatic "+"/"-" prefixing of packed output.

  This was originally done as a micro-optimization to keep packed
  output as short as possible for string packaged message formats.

  The optimization really doesn't make much difference, so isn't
  worth the loss in flexibility.
ptaoussanis added a commit that referenced this pull request Jun 1, 2022
@ptaoussanis
Copy link
Member

@rosejn Thanks for the quick response Jeff!

Merged and released on Clojars as [com.taoensso/sente "1.17.0-RC2"] 👍

Please note that the merged commit includes some changes to PR, notably:

  • You'll need to modify taoensso.sente/*write-legacy-pack-format?* in your case.
  • You'll need to include :ws-opts {:binary-type "arraybuffer"} in your options to make-channel-socket-client!.

Feedback welcome - changes can still be made before final v1.17.0 release.

@ptaoussanis ptaoussanis closed this Jun 1, 2022
ptaoussanis added a commit that referenced this pull request Mar 7, 2023
See `*write-legacy-pack-format?*` var docstring for full details.
ptaoussanis added a commit that referenced this pull request Mar 7, 2023
See `*write-legacy-pack-format?*` var docstring for full details.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants