Update to the latest version of cookie #1609
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is required for us to continue to make any changes to Cargo.toml, since all versions of ring except the most recent have been yanked. We only depend on ring through cookie. Moving from cookie 0.9 to 0.11 doesn't appear to require any code changes in either our code or conduit-cookie. We can't use `[patch]` here, since it will not force Cargo to resolve to a yanked version of a crate (it'll still just try to resolve conduit-cookie 0.8.4 off the index which inevitably depends on ring 0.11 so it'll fail and go back to the last version which used openssl)
|
@alexcrichton I have commit access to conduit-cookie, but I'm not able to publish a new version. Are you ok with me adding myself as an owner of that crate so I can publish a new version? |
|
Done! |
|
I've tested this in staging this looks good to me. My login cookie was still good and I did not need to log in again. @bors r+ |
|
📌 Commit 7412daa has been approved by |
bors
added a commit
that referenced
this pull request
Jan 31, 2019
Update to the latest version of cookie This is required for us to continue to make any changes to Cargo.toml, since all versions of ring except the most recent have been yanked. We only depend on ring through cookie. Moving from cookie 0.9 to 0.11 doesn't appear to require any code changes in either our code or conduit-cookie. We can't use `[patch]` here, since it will not force Cargo to resolve to a yanked version of a crate (it'll still just try to resolve conduit-cookie 0.8.4 off the index which inevitably depends on ring 0.11 so it'll fail and go back to the last version which used openssl)
|
☀️ Test successful - checks-travis |
bors
added a commit
that referenced
this pull request
Feb 1, 2019
Explicitly share a single connection in tests This is required to move forward on #1588, I have spun it off into a standalone PR for ease of review, since it's isolated and a change that makes sense on its own. This requires #1609 and includes those commits Right now the way our test suite works is that we set the database connection pool size to 1, and tell that single connection to start a test transaction. This works, but has the caveat that only a single reference to the connection can be live at any given time. This becomes a problem when we add background jobs into the mix, which will always want to grab two connections from the pool (and we still need them to be the same connection). The code that we end up running looks something like: ``` // In request let conn = pool.get(); insert_background_job(&conn)?; // In middleware that runs jobs for _ in 0..job_count { thread::spawn(|| { conn = pool.get(); lock_job(&conn); // in actual job conn = pool.get(); do_stuff(&conn); unlock_or_delete_job(&conn); }); } ``` Note that the worker and the job itself both need a connection. In order to make this work, we need to change our pool to actually just be a single connection given out multiple times in test mode. I've chosen to use a reentrant mutex for this for two reasons: 1. We need to satisfy the `Sync` bound on `App`, even though we're only running in a single thread in tests. 2. The background worker actually does end up running in another thread, so we do actually need to be thread safe. The result of this is that we can have the connection checked out more than once at the same time, but we still can't be more relaxed about when it gets dropped in our test code, since we need it to be unlocked if we try to get a connection from another thread as the result of running a background job. Our tests shouldn't know or care about whether background jobs were run async or not, so they should assume every request might run one. The mutex guard holds a reference to the mutex, so this introduces a lifetime constraint that hasn't existed since the Diesel port. The return type of `req.db_conn()` is now effectively `&PgConnection` instead of `Box<PgConnection>`. Since the method exists on `Request`, this means it gets treated as an immutable borrow of the whole request. The fixes are all pretty simple, and boil down to either cloning the app and getting the DB directly from there, taking immutable references instead of mutable ones, or dropping the connection when we're done with it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is required for us to continue to make any changes to Cargo.toml,
since all versions of ring except the most recent have been yanked. We
only depend on ring through cookie. Moving from cookie 0.9 to 0.11
doesn't appear to require any code changes in either our code or
conduit-cookie.
We can't use
[patch]here, since it will not force Cargo to resolve to ayanked version of a crate (it'll still just try to resolve
conduit-cookie 0.8.4 off the index which inevitably depends on ring 0.11
so it'll fail and go back to the last version which used openssl)