Fix API bug: Torrent cannot be whitelisted twice#117
Merged
Conversation
The API endpoint to whitelist torrents returns an error if you try to whitelist the same torrent twice. This test reproduces that wrong behavior before fixing it.
Member
Author
|
ACK 2d621c5 |
… errors 167b749 db: check info_hash record not found instead dropping all errors (Cameron Garnham) Pull request description: ACKs for top commit: josecelano: ACK 167b749 Tree-SHA512: 432a64371b0cc425ef21248c5fb7334257db30e6b52e8e0bebcb02291441523f1bddefe0780cd79693fe5e1cf26d3cf528d1b4e6d3eeebada32e108a2844aaa1
In the e2e tests we needed to wait until the API server is ready to accept request. We were waiting a random duration (100 milliseconds). Now we send a message from the API when is ready to the initiator. In production code is not used.
Member
Author
|
hi @da2ce7 @WarmBeer, I changed the implementation to send a message from the API server to the initiator when the API is ready, as we discussed yesterday. You can see the changes here. I do not like it yet. I think we should send the message after starting the server. I think we can do what @da2ce7 suggested. Something like: tokio::spawn(async move {
let (_addr, api_server) = serve(server).bind_with_graceful_shutdown(socket_addr, async move {
tokio::signal::ctrl_c().await.expect("Failed to listen to shutdown signal.");
});
});
// Send a message to the initiator to notify the API is ready to accept requests
if messenger_to_initiator.send(ApiReady()).is_err() {
panic!("the receiver dropped");
}@da2ce7 is that what you suggested? Using a new thread to launch the server and send the message afterwards. I'm not sure if that's a significant change. I suppose the problem was not the time it takes the server to be ready but the fact that tests run in parallel. |
There are two main changes: - The API server does not send the message when is ready. The job starter waits until the API server is running. This change is less radical becuase we keep the `start_job` return type as the other job starters. We did not want to send a real message from the API. We only wanted to know that the API thread is up and running. - The job starter waits until the API job is running even in production code. In the previous version we did that only for the e2e tests.
Member
Author
|
ACK 15aa831 |
da2ce7
reviewed
Nov 25, 2022
Cameron Garnham (@da2ce7) suggested this change. It's better to send the event after spwaning the API server task.
Member
Author
|
ACK 5274b2c |
da2ce7
approved these changes
Nov 25, 2022
josecelano
deleted the
issue-74-fix-api-response-for-duplicate-whitelist-requests
branch
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
should_allow_to_whitelist_a_torrentis_info_hash_whitelistedas described here.