Skip to content

Merge Conflicts for PR 'Error logging and examples' #284

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

Merged
merged 38 commits into from
Feb 4, 2021

Conversation

bahildebrand
Copy link
Contributor

@bahildebrand bahildebrand commented Feb 3, 2021

Issue #, if available: #242

Description of changes:
This PR is a continuation of #242. It solves the merge conflicts present from the branch becoming stale, and adds no new content. A new PR was created because we did not get a response from the owner of PR #242, and we did not have permissions to edit his branch. Credit to @rimutaka for the changes.

By submitting this pull request

  • [*] I confirm that my contribution is made under the terms of the Apache 2.0 license.
  • [*] I confirm that I've made a best effort attempt to update all relevant documentation.

davidbarsky and others added 30 commits July 17, 2020 17:30
# Conflicts resolved in favour of davidbarsky/update-readme:
#	Cargo.lock
#	lambda/Cargo.toml
#	lambda/examples/README.md
#	lambda/examples/basic.rs
#	lambda/examples/error-handling.rs
#	lambda/src/client.rs
#	lambda/src/lib.rs
rimutaka and others added 7 commits September 22, 2020 21:45
…mutaka/master`.

* origin/davidbarsky/update-readme:
  Removed unnecessary + Sync on handler's future
  Added docs and missing _vars to reduce warnings
  Enforced Send+Sync for http Handler impl.
  Added some tracing!() to lib.rs.
  Fixed "Process exited" error in lib.rs.
  client.rs formatting fix
  Log panic as Debug+Display, improved examples
  Added err logging to lib.rs, consolidated examples
  Added error handling for awslabs#241, interim, broken.
  disable time on `tracing_subscriber`
  Add log and tracing examples; fix `tracing` dependency features.
  Create a reusable runtime struct + builder
  Cleanup docs; handle panics in lambda functions correctly.
  Add Cargo.lock to gitignore; whoops.
  don't depend on serde_derive directly
  Replace `genawaiter` with `async-stream` (awslabs#240)
  reorg
@bahildebrand bahildebrand changed the title Rimutaka master Merge Conflicts for PR 'Error logging and examples' Feb 3, 2021
@bahildebrand bahildebrand marked this pull request as ready for review February 3, 2021 01:46
Copy link
Contributor

@coltonweaver coltonweaver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These look good to me.

@coltonweaver coltonweaver mentioned this pull request Feb 4, 2021
2 tasks
@bahildebrand bahildebrand merged commit e0803e1 into awslabs:master Feb 4, 2021
@vultix
Copy link

vultix commented Feb 9, 2021

@coltonweaver @rimutaka Is there a reason Send + Sync requirements were added? Sync is a restrictive requirement and breaks all of our lambdas using any of these:

  • Rusoto
  • Mongodb
  • Sqlx

This is the commit: Enforced Send+Sync for http Handler impl.

@bahildebrand
Copy link
Contributor Author

@vultix, what is failing exactly. Are you unable to pass in clients for those libraries to the invocations from the base lambda instance?

@vultix
Copy link

vultix commented Feb 10, 2021

@bahildebrand Here's a simple example. Note that normal lambdas work, it's only the http lambdas that have started failing:

use lambda_http::{
    handler,
    lambda::{self, Context},
    IntoResponse, Request, RequestExt, Response,
};

use serde_json::{json, Value};
use rusoto_core::Region;
use rusoto_dynamodb::{DynamoDb, DynamoDbClient, ListTablesInput};

type Error = Box<dyn std::error::Error + Send + Sync + 'static>;

#[tokio::main]
async fn main() -> Result<(), Error> {
    lambda::run(handler(func)).await?;
    Ok(())
}

async fn func(_: Request, _: Context) -> Result<Value, Error> {
    let client = DynamoDbClient::new(Region::UsEast1);

    let output = client.list_tables(Default::default()).await?;

    dbg!{output};

    Ok(json!({"success": true}))
}
[dependencies]
serde_json = "1.0"
serde = "1.0"
tokio = {version = "1.0", features = ["macros"]}

lambda_http = { git = "https://github.com/awslabs/aws-lambda-rust-runtime/", branch = "master"}
lambda = { git = "https://github.com/awslabs/aws-lambda-rust-runtime/", branch = "master"}
rusoto_dynamodb = "0.46.0"
rusoto_core = "0.46.0"

This is the error I get:

error: future cannot be shared between threads safely
   --> src/main.rs:16:17
    |
16  |     lambda::run(handler(func)).await?;
    |                 ^^^^^^^ future returned by `func` is not `Sync`
    | 
   ::: ~/.cargo/git/checkouts/aws-lambda-rust-runtime-7c865cce90132439/9bd45c1/lambda-http/src/lib.rs:109:19
    |
109 | pub fn handler<H: Handler>(handler: H) -> Adapter<H> {
    |                   ------- required by this bound in `handler`
    |
    = help: the trait `Sync` is not implemented for `dyn Future<Output = std::result::Result<ListTablesOutput, RusotoError<ListTablesError>>> + Send`
note: future is not `Sync` as it awaits another future which is not `Sync`
   --> src/main.rs:23:18
    |
23  |     let output = client.list_tables(Default::default()).await?;
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ await occurs here on type `Pin<Box<dyn Future<Output = std::result::Result<ListTablesOutput, RusotoError<ListTablesError>>> + Send>>`, which is not `Sync`

You get similar 'Sync' is not implemented errors when doing simple operations using mongodb and sqlx as well. If you pin lambda and lambda_http to a revision before this PR everything compiles.

@bahildebrand
Copy link
Contributor Author

@vultix Would you mind opening an issue for this so I can track it there? I'm going to take a look at this for you.

@vultix
Copy link

vultix commented Feb 12, 2021

@bahildebrand I created Issue #287

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.

6 participants