0

My javascript websocket client crashes when it is receiving messages at a high rate. I've tried 3 browsers and they all crash. Chrome as first, IE second and Edge can keep up a little longer.

The messagerate is about 30 messages in less then a second. All about 10-20 chars long (data-part).

Are the onmessage calls async? And can they run parallel (next onmessage already starts before the previous is done) or are the requests placed in some sort of queue?

I've tried to "block" the onMessage to no avail.

var blockWS = false;

function pageLoad() {
  if ('WebSocket' in window) {
    var ws = new WebSocket(((window.location.protocol === 'https:') ? 'wss://' : 'ws://') + window.location.host + '/ws');
    ws.onmessage = function(message) {
      while (blockWS) {
        // wait
      }
      blockWS = true;
      var el = document.getElementById('printer');
      el.innerHTML += message.data;
      el.scrollTop = el.scrollHeight;
      blockWS = false;
    };
  } else {
    // The browser doesn't support WebSocket
    document.getElementById('body').innerHTML = '<h1>Your browser does not support WebSocket.</h1>';
  }
}
<html>

<body onload="pageLoad()">
  <div id="body">
    <div id="printer"></div>
  </div>
</body>

</html>
BMelis
  • 384
  • 3
  • 16
  • Without performance-testing this code, I am going to assume that your browsers are crashing because you are trying to read and modify the same exact DOM element every single time that you receive a new message. Reading / writing to the DOM is EXTREMELY expensive and time-consuming in JavaScript. – th3n3wguy Mar 05 '18 at 16:12
  • @th3n3wguy You're probably* right. When I lower the message rate everything runs fine. (* because I didn't profile) – BMelis Mar 05 '18 at 18:49
  • By the way, the `blockWS` stuff isn't doing anything. I think you're trying to use it to prevent problems with two `onmessage` handlers trying to run at the same time. But this isn't a problem anyway, since the handlers aren't doing anything asynchronous. – David Knipe Mar 05 '18 at 21:13
  • Did you mean only 30 messages overall and it takes a second, or it keeps going forever at a rate of 30 messages per second? – David Knipe Mar 05 '18 at 21:18
  • the server is sending in bursts (with 30-60 secs in between bursts). But when it is sending, it's sending at 30/sec. So if it's not async, what happens if `ws` is trying to call the handler before the previous call is finished? – BMelis Mar 05 '18 at 21:37
  • 30 messages in the queue totalling <=600 bytes doesn't sound excessive to me, and neither does 60 messages per minute. If a message arrives while a javascript synchronous task is running, I think it will be added to the queue. If not, then I suppose it gets buffered somewhere further back; at worst it would force the connection to stay open while the sender waits for the backlog to clear, potentially causing a timeout after 30 seconds or whatever. But I don't think that would happen with data this small - it probably goes straight on the queue and gets handled soon afterwards. – David Knipe Mar 06 '18 at 22:27

1 Answers1

0

The easy (not sure about performance here) way to do what you're wanting to do is to use the setInterval() method outside of the loop and simply store the array of messages as they come in from the websockets. Here is some test code that you could do:

EDIT: I added an updated part of the code so that the insert into the DOM will only happen if there are new messages that need to be shown.

// Your code that is crashing the browser:
/*var blockWS = false;

function pageLoad() {
  if ('WebSocket' in window) {
    var ws = new WebSocket(((window.location.protocol === 'https:') ? 'wss://' : 'ws://') + window.location.host + '/ws');
    ws.onmessage = function(message) {
      while (blockWS) {
        // wait
      }
      blockWS = true;
      var el = document.getElementById('printer');
      el.innerHTML += message.data;
      el.scrollTop = el.scrollHeight;
      blockWS = false;
    };
  } else {
    // The browser doesn't support WebSocket
    document.getElementById('body').innerHTML = '<h1>Your browser does not support WebSocket.</h1>';
  }
}*/

// Using the setInterval method (I used 1s, but you can change that to your preferences):
function pageLoad() {
  const MESSAGE_INSERT_TIME_MS = 1000;
  let messages = [];
  let intervalId = null;
  
  if ('WebSocket' in window) {
    const protocol = window.location.protocol === 'https:' ? 'wss://' : 'ws://';
    const ws = new WebSocket(`${protocol}${window.location.host}/ws`);
    
    ws.onmessage = (message) => {
      messages.push(message);
    };
    
    // At any point inside the "pageLoad()" function, you can use "intervalId.clearInterval()" to stop doing this function.
    intervalId = setInterval(() => {
      const el = document.getElementById('printer');
      
      if (!el.innerHTML === messages.join('')) {
        el.innerHTML = messages;
        el.scrollTop = el.scrollHeight;
      }
    }, MESSAGE_INSERT_TIME_MS);
  }
}
th3n3wguy
  • 3,649
  • 2
  • 23
  • 30
  • But you continue updating the DOM every second, even when there are no more messages. That's not optimal. You could add a condition inside the `setInterval` handler to check whether an update is needed. Although I'd prefer to use https://lodash.com/docs/4.17.5#throttle (or rewrite `_.throttle` yourself if you're not using lodash). – David Knipe Mar 05 '18 at 21:09
  • This will eat a lot of memory, if running for too long. There is no need to keep the messages in the array after updating the DOM... – Marco Mar 06 '18 at 16:14
  • @d3L => Yes it will. As I stated originally, I am not sure how well it will perform in the long-term. The only reason I didn't clear out the messages is that there is a case where a message could be added to the messages array while the browser is reading from the DOM to get the current list of messages shown. – th3n3wguy Mar 06 '18 at 17:05
  • @th3n3wguy To give you a better idea, this is how I would've done it: https://pastebin.com/NCKvt2nM. – Marco Mar 06 '18 at 17:09
  • @d3L => Theoretically (not sure how often it would happen in a real-world scenario), but you could have the `onMessage()` listener that inserts a value into the `queue` array between the time you join those values and assign it to the `.innerHTML` and when you clear it out. That would technically run the possibility where you could have received a message and it would never get to the brower's DOM. That's the only reason I wrote the code the way I did. – th3n3wguy Mar 06 '18 at 17:13
  • 1
    @th3n3wguy Setting `innerHTML` is synchronous according to https://stackoverflow.com/questions/42986295/is-innerhtml-asynchronous so that should never happen. – Marco Mar 06 '18 at 17:18
  • @d3L => Yes. That operating is synchronous, but the `.scrollTop` is a layout-thrashing function that could force the browser to start rendering. Again, the possibility of my concern actually appearing are almost nothing, but it helps others to know of the possibilities. – th3n3wguy Mar 06 '18 at 17:22
  • 1
    @th3n3wguy Maybe, but the messages still wouldn't be lost. JavaScript is single threaded https://stackoverflow.com/questions/10517835/can-javascript-function-execution-be-interrupted – Marco Mar 06 '18 at 17:34