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

Update provided runtime to al2 #115

Merged
merged 14 commits into from Dec 17, 2020
Merged

Update provided runtime to al2 #115

merged 14 commits into from Dec 17, 2020

Conversation

hayd
Copy link
Contributor

@hayd hayd commented Nov 21, 2020

closes #114 , probably WIP (need to test on actual Lambda)

Must discuss how we push this change out as it's kinda breaking (if anyone is using other binaries in al1).
cc @lucacasonato @kyeotic.

@hayd
Copy link
Contributor Author

hayd commented Nov 21, 2020

Assets to test with provided.al2:
deno-lambda-example.zip
deno-lambda-layer.zip updated: deno-lambda-layer.zip

Edit: They seem to work for my initial tests on Lambda itself.


Note/Aside: I wonder if should publish the previous binaries in deno-docker (I know that at least one user was downloading these for centos7)...

@lucacasonato
Copy link
Member

@hayd Give me an hour - I'll try update deno.land/x to al2.

@hayd
Copy link
Contributor Author

hayd commented Nov 21, 2020

😬 😬 😬

@lucacasonato
Copy link
Member

denoland/deno_registry2#184

@lucacasonato
Copy link
Member

Registry went down with just updating to 1.5.2 on al1, so can't actually test right now. Sorry. denoland/deno_registry2#185

@hayd
Copy link
Contributor Author

hayd commented Nov 21, 2020

@lucacasonato do you think that is a deno-lambda bug or just /x ?

It looks like #113 ??

@lucacasonato
Copy link
Member

lucacasonato commented Nov 22, 2020

Looks like a deno bug. We shouldn't be trying to create the ts compiler cache if we are running a pure js file (bundle).

@hayd
Copy link
Contributor Author

hayd commented Nov 22, 2020

As mentioned in other issue, it's actually created in deno eval ! (I think it may well be a deno bug though.)
The investigate function is supposed to give helpful errors not red-herrings :)

@hayd
Copy link
Contributor Author

hayd commented Nov 22, 2020

Updated layer:
deno-lambda-layer.zip

@kyeotic
Copy link

kyeotic commented Nov 23, 2020

This does create better error messages, but the behavior is a bit strange. I added a throw before the handler is declared to mimic the error I was getting, and these are the logs that were produced. Note, this is a single invocation, not multiple as it appears


2020-11-23T08:48:10.539-08:00 | Check file:///tmp/runtime.js
-- | --
  | 2020-11-23T08:48:11.683-08:00 | error: Uncaught Error: test pre-handler failure
  | 2020-11-23T08:48:11.683-08:00 | throw new Error('test pre-handler failure')
  | 2020-11-23T08:48:11.683-08:00 | ^
  | 2020-11-23T08:48:11.683-08:00 | at api.ts:12:7
  | 2020-11-23T08:48:11.684-08:00 | /opt/bin/deno
  | 2020-11-23T08:48:11.730-08:00 | error: api.ts must export a function named 'handler'
  | 2020-11-23T08:48:11.813-08:00 | START RequestId: 746840fe-6d5a-4654-9aec-7adb7b6e9cfb Version: $LATEST
  | 2020-11-23T08:48:11.965-08:00 | Check file:///tmp/runtime.js
  | 2020-11-23T08:48:15.412-08:00 | error: Uncaught Error: test pre-handler failure
  | 2020-11-23T08:48:15.412-08:00 | throw new Error('test pre-handler failure')
  | 2020-11-23T08:48:15.412-08:00 | ^
  | 2020-11-23T08:48:15.412-08:00 | at api.ts:12:7
  | 2020-11-23T08:48:15.426-08:00 | /opt/bin/deno
  | 2020-11-23T08:48:19.129-08:00 | error: api.ts must export a function named 'handler'
  | 2020-11-23T08:48:19.166-08:00 | END RequestId: 746840fe-6d5a-4654-9aec-7adb7b6e9cfb
  | 2020-11-23T08:48:19.166-08:00 | REPORT RequestId: 746840fe-6d5a-4654-9aec-7adb7b6e9cfb Duration: 7352.27 ms Billed Duration: 7400 ms Memory Size: 512 MB Max Memory Used: 54 MB
  | 2020-11-23T08:48:19.166-08:00 | Unknown application error occurred Unhandled
  | 2020-11-23T08:48:20.175-08:00 | Check file:///tmp/runtime.js
  | 2020-11-23T08:48:21.132-08:00 | error: Uncaught Error: test pre-handler failure
  | 2020-11-23T08:48:21.132-08:00 | throw new Error('test pre-handler failure')
  | 2020-11-23T08:48:21.132-08:00 | ^
  | 2020-11-23T08:48:21.132-08:00 | at api.ts:12:7
  | 2020-11-23T08:48:21.134-08:00 | /opt/bin/deno
  | 2020-11-23T08:48:23.088-08:00 | error: api.ts must export a function named 'handler'

The response took 9.4seconds total.

@hayd
Copy link
Contributor Author

hayd commented Nov 23, 2020

@kyeotic The

  | 2020-11-23T08:48:11.684-08:00 | /opt/bin/deno

should be removed in the latest commit.

I think when you do this

I added a throw before the handler is declared

you get somewhat strange behavior from lambda if it errors on init (and IME it may run multiple times)... the fact it's outputting this /opt/bin/deno line multiple times suggests bootstrap is invocated multiple times.

I guess that message should say unable to import 'handler' from api.ts (in the cases where it compiles but has a runtime error)... and it should also be &> rather than 2> to hide any additional output.

We should add a file like that as a test case :)

Edit: Although note that this warn/error behavior isn't well tested - I thought there was an open issue...

@hayd
Copy link
Contributor Author

hayd commented Nov 23, 2020

For me, the following:

throw new Error("uh oh");

export async function handler(
  event: any,
  context: any,
) {
  console.log("here... somehow")
  return {
    statusCode: 200,
    headers: { "content-type": "text/html;charset=utf8" },
    body: `Hello from deno ${Deno.version.deno} 🦕!`,
  };
}

somehow kicks lambda to use an earlier version* of my function code, and runs that instead after the error! (If I comment out the throw then the expected code is run). I feel like this must be some esoteric edge case of lambda itself rather than deno-lambda.

Anyway, here is another update :)
deno-lambda-layer.zip

* maybe the first version? But certainly not the previous version. 🤷

@hayd
Copy link
Contributor Author

hayd commented Nov 23, 2020

Eurgh, it seems like the /runtime/init/error POSTing behavior has changed... Unless I am missing something... it used to be (and is on al1) that POSTing the /runtime/init/error would lead to Execution result: failed in the dashboard.

https://github.com/hayd/deno-lambda/blob/4c86198d81b0930eb51398c5629bc25ff875f76f/runtime/bootstrap#L57-L68

On al2 it doesn't... and somehow continues afterwards to successfully run another version of the function code. 🤮

... it seems implausible that curl would be doing something different, and that surely it's a lambda bug.

@hayd
Copy link
Contributor Author

hayd commented Nov 23, 2020

Okay, maybe it is less bad than I thought. Though still quite whacky...

My suspicion, though this makes no sense, is that in al2 IF your handler is the default hello.handler since you have another hello.ts in the layer then it'll try using that one on the second attempt?!!

It does seem to time out/spend a long time thinking in that case too. When I renamed to foo.ts I couldn't reproduce (it worked immediately as on al1). Also minimum usecase (without any layers but with bootstrap posting to /init/error) I was unable to reproduce - it works as expected.

I wonder if adding --no-check would be wise (and speed up the failures).

🤷

Edit: And now I can't replicate it at all!!

@kyeotic
Copy link

kyeotic commented Nov 23, 2020

--no-check seems like it would be wise. The safety checks should have already occured, whether your used deno cache or deno bundle, the code should be ready to go.

I don't like the idea of shipping the hello modules in the layer, especially if they can be used in edfe case scenarios. I don't want "hello world" showing up in an API response.

@hayd
Copy link
Contributor Author

hayd commented Dec 2, 2020

I guess we'll release this with 1.6.1 (assuming 1.6.0 is the first release to fix denoland/deno#8584 )

@hayd
Copy link
Contributor Author

hayd commented Dec 8, 2020

1.6.0 is released with al1.

I've updated this branch to 1.6.0 as well and here is the zip:
deno-lambda-layer.zip


The tests are failing due to ts error in the serverless-example (a hack with unknown apparently no longer works!) 🤔

@kyeotic
Copy link

kyeotic commented Dec 8, 2020

I tested out the layer and it works, but the performance has gotten noticeably worse. The runtime overhead is up to ~400ms, even on a warm lambda

@hayd
Copy link
Contributor Author

hayd commented Dec 8, 2020

@kyeotic What are/were you seeing on al1?

I wonder what it could be. Can building the deno binary from scratch make any difference (I doubt it)?
Could it be the --no-check ? There's not really other changes that would explain it... aside from the al2 runtime itself.

@kyeotic
Copy link

kyeotic commented Dec 8, 2020

I was seeing around ~250ms overhead on a warm lambda, which is still pretty bad.

For comparison previously testing has shown a bare NodeJS lambda can cold-start in 150ms and has <10ms overhead when warm.

I've never done any other perf testing on layers, so it may be a lambda layer overhead.

This feels a bit hacky...
@hayd
Copy link
Contributor Author

hayd commented Dec 16, 2020

I am going to bump this to 1.6.1 and ship in the next couple of days.

I don't have any insight into why things might be slower, and it's unfortunate, but we are getting very close to al1 EOL.

@hayd hayd merged commit 2c5ef70 into master Dec 17, 2020
@hayd hayd deleted the amazonlinux2 branch December 17, 2020 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update deno-lambda to use Amazon Linux 2
3 participants