0

So I have narrowed down the issue with my script.

the issue is this line

clients.push(res);

What it seems to be doing is firing 2 / 3 times instead of 1.

in the console.log it is showing:

STATION_ID Completed
754542
get data connected!
undefined
STATION_ID Stream END
get POdcast ran
STATION_ID Completed
754542
get data connected!
undefined
STATION_ID Stream END
get POdcast ran

however when I remove the res from clients.push(res); it fires normally but does not return in the browser to the client.

Any suggestions?

full code:

    router.get('/track/:url(*)', (req, res) =>{
    var url = req.params.url.substr(0); 
    console.log('/track/'+url);

    var length = 0;
    var e = 0;
    /* AD SYS */
    var remote = "https://storage.googleapis.com/ad-system/testfolder/OUTOFAREA.mp3";  
    var adsys = needle.get(remote)
    /* PODCAST */
    var filesize = needle.get(url, function(error) {
       if(error){
        e = 505;
        res.send("<html><head></head><body>NOPE</body></html>");
        console.log(error)
        //filesize.end();
        res.end();

       }
    });


        adsys.on('response', function(resB) {
            console.log("STATION_ID Completed");
            length =  Number(resB.headers['content-length']);
           // console.log(length);           
        });

        filesize.on('response', function(resC) {
                console.error("get data connected!");
                console.log(resC.headers['content-length']);
                a = Number(resC.headers['content-length']);
                length = length+a;

      });
      res.set({
        "Content-Type": "audio/mpeg",
        'Transfer-Encoding': 'chunk',
    //    'Content-Disposition': 'attachment',
    //    'Content-Length':length
    });

      adsys.on("finish", function() {

        console.log(" X STATION_ID Stream END");

           getPodcast();



    });  


    adsys.on("data", function (chunk) {
            // console.log(clients.length);
           /* if (clients.length > 0){
                for (client in clients){
                    clients[client].write(chunk);
                    //console.log(chunk);
                };
            }*/
            res.write(chunk);

    });

    function getPodcast(){


        filesize.on("data", function (chunk) {
           res.write(chunk);
        });

        filesize.on('end', function(){
            console.log("end");
            res.end();
        });
    }
});

module.exports = router;
RussellHarrower
  • 6,470
  • 21
  • 102
  • 204
  • What is the point of `clients.push(res);`? And, where are you actually sending a response back to the client that isn't an error response? And, do you realize that you created a race between the adsys and filesize requests with no synchronization between the two? It doesn't seem like there's any way that can work. – jfriend00 Mar 05 '20 at 06:17
  • Also, never iterate an array like `for (client in clients) {}`. See [this answer](https://stackoverflow.com/questions/22754315/for-loop-for-htmlcollection-elements/22754453#22754453) for a discussion. In today's node.js, you can/should use `for (client of clients) {}`. But, it does not appear there's any reason for the `clients` array in this code anyway. – jfriend00 Mar 05 '20 at 06:23
  • After you call `res.write()`, you have to call `res.end()`. The browser will not process the response it receives until you call `res.end()`. – jfriend00 Mar 05 '20 at 06:23
  • @jfriend00 thanks for the comments but res.end() where do you want me to put it because that is not the issue with the code. from my testing its the `clients.push(res);` and the res part of that code. also when I change `clients.push(res) to clients.push(res.end())` it crashes. also `client of clients` does not work for this example – RussellHarrower Mar 05 '20 at 06:48
  • Why are you using the `clients` array at all? As best I can tell, the code you show only ever puts one `res` into the array. You call `res.end()` AFTER you call `res.write()`. – jfriend00 Mar 05 '20 at 06:48
  • @jfriend00 I am trying to stitch two mp3 files together - it works but on server side seems to log it twice. https://api.drn1.com.au:9000/podcast/track/https://storage.googleapis.com/radiomediapodcast/wellwellnow/season1/S01E04.mp3 – RussellHarrower Mar 05 '20 at 06:49
  • Well, when you use `for (client of clients)`, you change what `client` is. It's already the array item. I'd really suggest you learn the newer Javascript features like `for/of`. `for/in` is NEVER for iterating array elements. Problems can occur. – jfriend00 Mar 05 '20 at 06:49
  • But, you only ever show pushing ONE item into the `clients` array so it seems pointless to me. Is there code missing from what you posted? – jfriend00 Mar 05 '20 at 06:50
  • Your code would be less confusing if you only had one variable named `res`. I'm not honestly sure which one you mean to be using in some places. – jfriend00 Mar 05 '20 at 06:53
  • @jfriend00 sorry about the res issue - I changed the original code - to better fix that – RussellHarrower Mar 05 '20 at 06:58

1 Answers1

0

OK, now you have more of the whole code so I can see what you're trying to accomplish.

A few issues I see:

  1. The variable a is not defined, so it's an implicit global. That's bad. Declare it somewhere local.
  2. When computing the combined content-length, you have a complete race condition between your two requests. Your code is assuming that adsys gives you a response first, but that is not guaranteed to happen.
  3. You compute that length, but don't actually use it. You can't put it in your res.set() instruction because you haven't necessarily finished calculating it yet when that runs.
  4. It seems like you're missing error handling on at least the adsys request.
  5. It appears to me in the needle documentation that the completion event for a needle request is "done", not "finish".

Here's a significantly cleaned up version of your code with a bunch of new logging to help debug:

let trackCntr = 0;
function getLogger() {
    let id = ++trackCntr;
    return function(...args) {
        args.unshift(`(${id}): `);
        console.log(...args);
    }
}

router.get('/track/:url(*)', (req, res) => {
    const log = getLogger();
    const url = req.params.url.substr(0);
    log(`(${trackCntr}) /track/${url}`);

    /* AD SYS */
    const remote = "https://storage.googleapis.com/ad-system/testfolder/OUTOFAREA.mp3";
    const adsys = needle.get(remote);

    /* PODCAST */
    const filesize = needle.get(url);

    let responseCntr = 0;
    let length = 0;
    let errReportedAlready = false;

    adsys.on("response", function(resB) {
        log(`adsys.on('response'), content-length=${resB.headers['content-length']}`);
        length += +resB.headers['content-length'];
        ++responseCntr;
        checkResponseCntr();
    });

    adsys.on("err", sendErr);
    adsys.on("timeout", sendErr);

    filesize.on("response", function(resC) {
        log(`filesize.on('response'), content-length=${resC.headers['content-length']}`);
        length += +resC.headers['content-length'];
        ++responseCntr;
        checkResponseCntr();
    });

    filesize.on("err", sendErr);
    filesize.on("timeout", sendErr);

    // this is called if either needle requests gets an error
    function sendErr(err) {
        log("sendErr", err);
        if (!errReportedAlready) {
            errReportedAlready = true;
            if (res.headersSent) {
                // just need to abort the response because headers have already been sent
                res.end();
            } else {
                // send error status
                res.sendStatus(500);
            }
        }
    }


    // code continues here after both response headers above have completed
    function checkResponseCntr() {
        log(`checkResponseCntr(${responseCntr})`)
        // if we have received both responses, then start streaming ad data
        if (responseCntr === 2) {
            log("got both responses");
            res.set({
                "Content-Type": "audio/mpeg",
                "Transfer-Encoding": "chunk",
                "Content-Length": length
            });

            // start streaming ad data
            getAd();

        }
    }

    function getAd() {
        log("getAd()");

        // this will cause adsys data to start flowing
        adsys.on("data", function(chunk) {
            if (!errReportedAlready) {
                res.write(chunk);
            }
        });

        adsys.on("done", function() {
            log("adsys done");
            // now trigger getting the podcast data
            getPodcast();
        });
    }

    function getPodcast() {
        log("getPodcast()");
        filesize.on("data", function(chunk) {
            if (!errReportedAlready) {
                res.write(chunk);
            }
        });

        filesize.on("done", function() {
            log("filesize done");
            if (!errReportedAlready) {
                log("res.end()")
                res.end();
            }
        });
    }
});

module.exports = router;

This does the following:

  1. Correctly compute the length without regard to race condition order of the two requests.
  2. Add error handling for both needle() requests.
  3. Add proper setting of content-length header.
  4. Change monitoring of needle() complete to use the "done" event per the needle() documentation.
  5. Code getAd() and getPodcast() similarly.

Possible issues still:

  1. If it takes a long time to stream the ad, I could imagine your filesize request timing out.

I'm able to run the above code in my own little nodejs app and this is the logging I get:

(1):  (1) /track/https://storage.googleapis.com/radiomediapodcast/wellwellnow/season1/S01E04.mp3
(1):  adsys.on('response'), content-length=754542
(1):  checkResponseCntr(1)
(1):  filesize.on('response'), content-length=63062853
(1):  checkResponseCntr(2)
(1):  got both responses
(1):  getAd()
(1):  adsys done
(1):  getPodcast()
(2):  (2) /track/https://storage.googleapis.com/radiomediapodcast/wellwellnow/season1/S01E04.mp3
(2):  adsys.on('response'), content-length=754542
(2):  checkResponseCntr(1)
(2):  filesize.on('response'), content-length=63062853
(2):  checkResponseCntr(2)
(2):  got both responses
(2):  getAd()
(2):  adsys done
(2):  getPodcast()
(2):  filesize done
(2):  res.end()
(1):  filesize done
(1):  res.end()

You can clearly see the two separate requests coming in. The second request arrives as soon as the first request sends it's headers - not sure why.


I've determined that the double request is caused by putting the audio URL into the Chrome URL bar. If I put that URL into an audio tag in an HTML page, then we no longer get the double requests. I created this simple HTML page:

<!DOCTYPE html>

<html xmlns="http://www.w3.org/1999/xhtml">
<head>
    <title></title>
</head>
<body>
    <figure>
        <figcaption>Listen to the T-Rex:</figcaption>
        <audio
            controls
            src="http://localhost/track/https%3A%2F%2Fstorage.googleapis.com%2Fradiomediapodcast%2Fwellwellnow%2Fseason1%2FS01E04.mp3">
                Your browser does not support the
                <code>audio</code> element.
        </audio>
    </figure>
</body>
</html>

And, then I get just this log (which looks right to me):

(1):  (1) /track/https://storage.googleapis.com/radiomediapodcast/wellwellnow/season1/S01E04.mp3
(1):  Accept: */*
(1):  adsys.on('response'), content-length=754542
(1):  checkResponseCntr(1)
(1):  filesize.on('response'), content-length=63062853
(1):  checkResponseCntr(2)
(1):  got both responses
(1):  getAd()
(1):  adsys done
(1):  getPodcast()
(1):  filesize done
(1):  res.end()

And, the double request seems to be a Chrome/Edge URL bar thing because it does not happen in Firefox when I put the URL into the Firefox URL bar.

jfriend00
  • 683,504
  • 96
  • 985
  • 979
  • @RussellHarrower - I added a cleaned up version of your code to my answer. – jfriend00 Mar 05 '20 at 07:40
  • @RussellHarrower - Did this help you? – jfriend00 Mar 05 '20 at 23:45
  • it still produced the same error: console.log returned the following 2 times. ```/track/https://storage.googleapis.com/radiomediapodcast/wellwellnow/season1/S01E04.mp3 get data connected! 63062853 STATION_ID Completed X STATION_ID Stream END /track/https://storage.googleapis.com/radiomediapodcast/wellwellnow/season1/S01E04.mp3 end STATION_ID Completed get data connected! 63062853 X STATION_ID Stream END end``` – RussellHarrower Mar 06 '20 at 03:31
  • @RussellHarrower - I modified the code in my answer to add a lot more debugging. If you can share the console log from that, it would help. It also appears that something is hitting your route more than once. – jfriend00 Mar 06 '20 at 04:36
  • @RussellHarrower - Other than the double request, it actually seems to be working for me. I put the above code from my answer into a small little node.js app and I can run it just fine here. With the new debugging, you can clearly see the logging from the two separate requests. – jfriend00 Mar 06 '20 at 07:36
  • @RussellHarrower - Double requests only seem to happen if you put the audio URL into the browser URL bar, not if you use an ` – jfriend00 Mar 06 '20 at 08:15
  • Thanks for that information. so could to solve this could i disallow direct requests if they come from chrome/Edge (chromium) but allow it if its in an audio player? – RussellHarrower Mar 06 '20 at 10:15
  • @RussellHarrower - I don't know how you would disable direct requests. You would have to guess whether it was a direct request or not. – jfriend00 Mar 06 '20 at 16:37