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

2.0 seems to have broken static file serving on azure. #21

Open
CyrusNajmabadi opened this issue Sep 7, 2018 · 12 comments
Open

2.0 seems to have broken static file serving on azure. #21

CyrusNajmabadi opened this issue Sep 7, 2018 · 12 comments
Labels

Comments

@CyrusNajmabadi
Copy link
Contributor

I have the following code in my function-app:

app.use("/", express.static("www"));

This results in the following runtime exception:

2018-09-07T22:14:29.829 [Info] Function started (Id=65d1f81f-f1ed-4d9f-844e-7fdcae8cf5c6)
2018-09-07T22:14:31.782 [Error] A ScriptHost error has occurred
2018-09-07T22:14:31.782 [Error] System.InvalidOperationException : TypeError: Cannot read property 'length' of null
    at ServerResponse.OutgoingMessage._send (_http_outgoing.js:137:38)
    at ServerResponse.OutgoingMessage.write (_http_outgoing.js:491:16)
    at ReadStream.ondata (_stream_readable.js:555:20)
    at emitOne (events.js:96:13)
    at ReadStream.emit (events.js:188:7)
    at readableAddChunk (_stream_readable.js:176:18)
    at ReadStream.Readable.push (_stream_readable.js:134:10)
    at onread (fs.js:2018:12)
    at FSReqWrap.wrapper [as oncomplete] (fs.js:682:17)

Note the calls into the new methods inherited by OutgoingMessage.

I'm not sure what's up here but i think this starting breaking when the change to derive from NativeOutgoingMessage. Do you know what's up @yvele ? Do you have a recommendation on what can be used instead?

@CyrusNajmabadi
Copy link
Contributor Author

CyrusNajmabadi commented Sep 7, 2018

The crashing line in question appears to be: https://github.com/nodejs/node/blob/1cb5aed5f596a9cb5b68e3a676222c0c35501a5e/lib/_http_outgoing.js#L137

    } else {
      this.output.unshift(this._header);
      this.outputEncodings.unshift('latin1');
      this.outputCallbacks.unshift(null);
      this.outputSize += this._header.length;  //<-- this line.
      if (typeof this._onPendingData === 'function')
        this._onPendingData(this._header.length);
}

@CyrusNajmabadi
Copy link
Contributor Author

@yvele I've been trying my best to debug through this and figure out what the right thing to do is. but i keep hitting a brick wall. The node http stack is just insanely complicated. I've tried as well on 10.6 but this is broken there as well.

If you could help out here at all it would be very appreciated. Thanks!

@CyrusNajmabadi
Copy link
Contributor Author

@maktwin Do you have any ideas either? Do either of you two use static file serving in your FunctionApps? If so, do you do anything special to make it work?

@yvele yvele added the bug label Sep 8, 2018
@yvele
Copy link
Owner

yvele commented Sep 8, 2018

Diagnostic

I've overwritten the internal writeHead method to setup context.res.headers (specific to Azure Functions) but it's specific to node 6.

/**
* https://nodejs.org/api/http.html#http_response_writehead_statuscode_statusmessage_headers
* Original implementation: https://github.com/nodejs/node/blob/v6.x/lib/_http_server.js#L160
*
* @param {Object} context Azure Function context
* @param {number} statusCode
* @param {string} statusMessage
* @param {Object} headers
* @this {OutgoingMessage}
*/
function writeHead(context, statusCode, statusMessage, headers) {

Node 8~10 doesn't have this internal writeHead method anymore 🤔

PS: By the way this may be an inspiration https://github.com/awslabs/aws-serverless-express

Possible solution

By inheriting native OutgoingMessage we maybe can skip manually implementing writeHead and maybe we can do something like:

constructor(context) {
    super();
    this.context = context; // Azure Function context
}

// Override by adding things specific to Azure Functions runtime
end(data, encoding) {
  super.end(data, encoding);

  // Return raw values to Azure Function runtime
  const { context } = this;
  context.res.headers = ...;
  context.res.status = ...;
  context.res.body = convertToBody(data, encoding);
  context.res.isRaw = true;
  context.done();
}

We also need unit tests for that (covering all supported node versions).

@CyrusNajmabadi
Copy link
Contributor Author

Interesting info. Note: i've been using aws-serverless-express very successfully on aws itself. I was going to try something today which first seemed odd, but now i think would b ea good idea. Specifically, i was going to just trying using aws-serverless-express on azure. It's just a library which spawns a local http server, then sends an http request to it. It does expect to take an APIGatewayRequest+serverless.Context as arguments, and it will return produce different result values than azure needs. However, i think i can map azure's inputs to the inputs it needs, and i think i can map its outputs to the ones that azure needs as well.

By actually talking to a real node http server, there is no need to try to simulate an OutgoingMessage/ServerResponse. Instead, the true node will be used. That should hopefully make it so all middleware (like express.static) works fine. All that needs to happen is to properly marshal between the azure and aws input/output types, which i think is very doable...

@CyrusNajmabadi
Copy link
Contributor Author

I got this all working by moving over to aws-serverless-express. Turns out it works fine as a libary on azure. The code to get it to work was pretty simple, and i can link to it once it's cleaned up.

@CyrusNajmabadi
Copy link
Contributor Author

PR that switches to just using aws-serverless-express: pulumi/pulumi-cloud#584

We're also considering extracting this into a standalone library . Let me know if you'd be interested in that!

@lworkman
Copy link

That's for the tip @CyrusNajmabadi , I found a different library that implemented aws-serverless-express for azure and it worked like a charm (https://github.com/bigx333/azure-aws-serverless-express).

@yvele I do really like this library, and would switch back once this issue gets fixed. To give you more details, I was trying to serve a ReadableStream from azure blob storage through express and kept running into the headers issue.

@kfhak-zb
Copy link

kfhak-zb commented Feb 20, 2019

maybe it's related in "OutgoingMessage.js"

I changed "OutgoingMessage.js" like below

function convertToBody(body, encoding) {

// return Buffer.isBuffer(body) ? body.toString(encoding) : body; //origin code
return Buffer.isBuffer(body) && "binary" != encoding ? body.toString(encoding) : body;

}

and test like this

app.get("/api/getfile", function (req, res) {

const data = fs.readFileSync("sample.pdf");

res.writeHead(200, {

	"Content-Type": "application/pdf",

	"Content-Disposition": "attachment;filename=output.pdf"

});

res.end(data, "binary");

});

It downloaded the file without "TypeError: Cannot read property 'length' of null"

@franksmule
Copy link

Same problem, checking the encoding like @kfhak-zb does solves the issue

@gunzip
Copy link

gunzip commented Jan 17, 2020

any update on this ?

@wpitallo
Copy link

Anyone get this working?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants