From d5f33a9a986f21c8b660eb1d3faa9953f2b8bba7 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Tue, 14 Nov 2017 23:24:04 +0100 Subject: find_and_track_build_and_update_status(): Try to wrap the fn in a thread Try to put the whole function inside a thread. This doesn't work because of move problems. The reason for wrapping things in a thread is so I can add a `sleep` at the top of the function. We want to sleep before even making our first request to Jenkins because it seems like we're making the request too early, before the job has been created. This prevents the GitHub status from being set. --- src/jenkins.rs | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) (limited to 'src') diff --git a/src/jenkins.rs b/src/jenkins.rs index 61d3bf1..d507f00 100644 --- a/src/jenkins.rs +++ b/src/jenkins.rs @@ -97,17 +97,28 @@ pub fn find_and_track_build_and_update_status( jenkins_user_id: &String, jenkins_token: &String, github_token: String, -) -> Result<(), Box> { +// ) -> Result<(), Box> { +// ) -> thread::JoinHandle { +) -> thread::JoinHandle<()> { + thread::spawn(move || { + // Wait for Jenkins to warm up + sleep(Duration::from_secs(10)); + let jenkins_client = jenkins_request_client( &jenkins_user_id, &jenkins_token - )?; + ).expect("Failed to initialise Jenkins HTTP client"); let jobs = get_jobs( &jenkins_url, &jenkins_client, commit_ref.repo.as_ref() - )?; + ).expect( + format!( + "Failed to request Jenkins jobs for {}", + commit_ref.repo + ).as_ref() + ); let t20_minutes = 60 * 20; for job_url in jobs { @@ -115,7 +126,9 @@ pub fn find_and_track_build_and_update_status( &jenkins_url, &jenkins_client, job_url.as_ref() - )?; + ).expect( + format!("Failed to request Jenkins job {}", job_url).as_ref() + ); // Does `displayName` match if job_for_commit(&job, &commit_ref) { @@ -206,11 +219,13 @@ pub fn find_and_track_build_and_update_status( } }); - return Ok(()) + // return Ok(()) + return } } + }) - Ok(()) + // Ok(()) } pub fn auth_credentials(user_id: String, token: String) -> header::Basic { -- cgit v1.2.3 From f9b13604484fa7924efb8cde0dd26de0276065ff Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Tue, 14 Nov 2017 23:46:27 +0100 Subject: find_and_track_build_and_update_status(): Move thread outside function Instead of wrapping the inside of the function in a thread, put the thread on the outside, in `main()`. This is an effort to try to get around the `move`/lifetime issues I was having with the previous version. Add some extra logging so we have a better idea what's going on when. Remove the `expects` I had added and put the `Result` returns back. In `main()`, wrap the call to `find_and_track_build_and_update_status()` in a thread and sleep before calling it. This allows us to give Jenkins some time to warm up and create a job for our commit before we go ahead and try to request it from its API. Otherwise, if we kick off the Jenkins fetch too soon, our statuses aren't going to get updated because the job won't have been created and thus won't have been found on the `get_jobs()` call. Had to add some ugly `clone()`s to get around `move` compiler errors here. Next step is figuring out how to clean that up. --- src/jenkins.rs | 57 ++++++++++++++++++++++++++++----------------------------- src/main.rs | 40 ++++++++++++++++++++++++++-------------- 2 files changed, 54 insertions(+), 43 deletions(-) (limited to 'src') diff --git a/src/jenkins.rs b/src/jenkins.rs index d507f00..d61e7d0 100644 --- a/src/jenkins.rs +++ b/src/jenkins.rs @@ -43,7 +43,6 @@ extern crate reqwest; extern crate url; use std::error::Error; -use std::thread; use std::thread::sleep; use std::time::{Duration, Instant}; @@ -93,46 +92,46 @@ impl Job { pub fn find_and_track_build_and_update_status( commit_ref: CommitRef, - jenkins_url: String, - jenkins_user_id: &String, - jenkins_token: &String, - github_token: String, -// ) -> Result<(), Box> { -// ) -> thread::JoinHandle { -) -> thread::JoinHandle<()> { - thread::spawn(move || { - // Wait for Jenkins to warm up - sleep(Duration::from_secs(10)); + // jenkins_url: String, + // jenkins_user_id: &String, + // jenkins_token: &String, + // github_token: String, + jenkins_url: &str, + jenkins_user_id: &str, + jenkins_token: &str, + github_token: &str, +) -> Result<(), Box> { + // TODO: Remove this, rest should take same type as arguments maybe + let jenkins_url = jenkins_url.to_owned(); + let jenkins_user_id = &jenkins_user_id.to_owned(); + let jenkins_token = &jenkins_token.to_owned(); + let github_token = github_token.to_owned(); let jenkins_client = jenkins_request_client( &jenkins_user_id, &jenkins_token - ).expect("Failed to initialise Jenkins HTTP client"); + )?; let jobs = get_jobs( &jenkins_url, &jenkins_client, commit_ref.repo.as_ref() - ).expect( - format!( - "Failed to request Jenkins jobs for {}", - commit_ref.repo - ).as_ref() - ); + )?; let t20_minutes = 60 * 20; for job_url in jobs { + info!("Looking for job: {}", job_url); + let mut job = request_job( &jenkins_url, &jenkins_client, job_url.as_ref() - ).expect( - format!("Failed to request Jenkins job {}", job_url).as_ref() - ); + )?; // Does `displayName` match if job_for_commit(&job, &commit_ref) { - thread::spawn(move || { + info!("Job found: {}", job_url); + // thread::spawn(move || { // Start timer let now = Instant::now(); @@ -165,6 +164,8 @@ pub fn find_and_track_build_and_update_status( // call github::update_commit_status // stop + info!("Waiting for job to finish"); + if now.elapsed().as_secs() == t20_minutes { github::update_commit_status( &github_token, @@ -182,7 +183,7 @@ pub fn find_and_track_build_and_update_status( ).as_ref() ); - return + return Ok(()) } sleep(Duration::from_secs(30)); @@ -212,20 +213,18 @@ pub fn find_and_track_build_and_update_status( ).as_ref() ); - return + return Ok(()) } job = updated_job; } - }); + // }); - // return Ok(()) - return + return Ok(()) } } - }) - // Ok(()) + Ok(()) } pub fn auth_credentials(user_id: String, token: String) -> header::Basic { diff --git a/src/main.rs b/src/main.rs index 05ca775..c93d616 100644 --- a/src/main.rs +++ b/src/main.rs @@ -25,7 +25,9 @@ extern crate rouille; extern crate kipper; use std::env; +use std::thread; use std::io::Read; +use std::time::Duration; use getopts::Options; @@ -137,20 +139,30 @@ fn main() { }, }; - match jenkins::find_and_track_build_and_update_status( - commit_ref, - jenkins_url.clone(), - &jenkins_user_id, - &jenkins_token, - github_token.clone(), - ) { - Ok(_) => {}, - Err(e) => { - error!("{}", e.to_string()); - - return internal_server_error() - }, - }; + // TODO: Get rid of clones + let jenkins_url = jenkins_url.clone(); + let jenkins_user_id = jenkins_user_id.clone(); + let jenkins_token = jenkins_token.clone(); + let github_token = github_token.clone(); + + thread::spawn(move || { + thread::sleep(Duration::from_secs(30)); + + match jenkins::find_and_track_build_and_update_status( + commit_ref, + &jenkins_url, + &jenkins_user_id, + &jenkins_token, + &github_token, + ) { + Ok(_) => {}, + Err(e) => { + error!("{}", e.to_string()); + + // return internal_server_error() + }, + }; + }); rouille::Response::text("202 Accepted") .with_status_code(202) -- cgit v1.2.3 From 84d4e100e93a5e3b630eaa355120871af46667d2 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Thu, 16 Nov 2017 23:39:15 +0100 Subject: main(): Add comment above cloned command line variables Turns out the easiest way to share these variables between both closures really is to clone them. Thanks to 'durka42' on Mozilla#rust-beginners for explaining the problem to me. In durka's words: > both closures are required to be 'static > so you can't send references across > and the router handler has to be callable multiple times so you can't consume anything (that's the error you got) It's also possible to share references using Arc, but that seems more complicated than it's worth here. Just do the copy and be done with it. --- src/main.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'src') diff --git a/src/main.rs b/src/main.rs index c93d616..8c4fbfb 100644 --- a/src/main.rs +++ b/src/main.rs @@ -139,7 +139,9 @@ fn main() { }, }; - // TODO: Get rid of clones + // Clone so we can use these values in the thread + // closure. Since both closures are required to be + // 'static, we can't use references to these values. let jenkins_url = jenkins_url.clone(); let jenkins_user_id = jenkins_user_id.clone(); let jenkins_token = jenkins_token.clone(); -- cgit v1.2.3 From 90ef027f2f36a8cb321b2dcaa8a411425338c200 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Thu, 16 Nov 2017 23:45:36 +0100 Subject: find_and_track_build_and_update_status(): Restore old arg types I had changed these when I was having trouble passing the values through my double-closure, but now that that's settles, we can revert these types to the way they were before and get rid of the unnecessary `to_owned()` transformations. --- src/jenkins.rs | 18 ++++-------------- src/main.rs | 4 ++-- 2 files changed, 6 insertions(+), 16 deletions(-) (limited to 'src') diff --git a/src/jenkins.rs b/src/jenkins.rs index d61e7d0..038056a 100644 --- a/src/jenkins.rs +++ b/src/jenkins.rs @@ -92,21 +92,11 @@ impl Job { pub fn find_and_track_build_and_update_status( commit_ref: CommitRef, - // jenkins_url: String, - // jenkins_user_id: &String, - // jenkins_token: &String, - // github_token: String, - jenkins_url: &str, - jenkins_user_id: &str, - jenkins_token: &str, - github_token: &str, + jenkins_url: String, + jenkins_user_id: &String, + jenkins_token: &String, + github_token: String, ) -> Result<(), Box> { - // TODO: Remove this, rest should take same type as arguments maybe - let jenkins_url = jenkins_url.to_owned(); - let jenkins_user_id = &jenkins_user_id.to_owned(); - let jenkins_token = &jenkins_token.to_owned(); - let github_token = github_token.to_owned(); - let jenkins_client = jenkins_request_client( &jenkins_user_id, &jenkins_token diff --git a/src/main.rs b/src/main.rs index 8c4fbfb..211a680 100644 --- a/src/main.rs +++ b/src/main.rs @@ -152,10 +152,10 @@ fn main() { match jenkins::find_and_track_build_and_update_status( commit_ref, - &jenkins_url, + jenkins_url, &jenkins_user_id, &jenkins_token, - &github_token, + github_token, ) { Ok(_) => {}, Err(e) => { -- cgit v1.2.3 From 4f8f9fc7497d62385cc4d82c36892587703352f0 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Thu, 16 Nov 2017 23:48:38 +0100 Subject: find_and_track_build_and_update_status(): Remove commented thread::spawn This `thread::spawn()` call was moved outside the function and into `main()`. We can remove it now. --- src/jenkins.rs | 161 ++++++++++++++++++++++++++++----------------------------- 1 file changed, 80 insertions(+), 81 deletions(-) (limited to 'src') diff --git a/src/jenkins.rs b/src/jenkins.rs index 038056a..273e26c 100644 --- a/src/jenkins.rs +++ b/src/jenkins.rs @@ -121,94 +121,93 @@ pub fn find_and_track_build_and_update_status( // Does `displayName` match if job_for_commit(&job, &commit_ref) { info!("Job found: {}", job_url); - // thread::spawn(move || { - // Start timer - let now = Instant::now(); - - let commit_status = job.result.commit_status(); - - github::update_commit_status( - &github_token, - &commit_ref, - &commit_status, - job_url.clone(), - None, - "continuous-integration/jenkins".to_owned() - ).expect( - format!( - "GitHub pending status update failed for {}/{} {}.", - commit_ref.owner, - commit_ref.repo, - commit_ref.sha - ).as_ref() - ); - while job.result == JobStatus::Pending { - // loop - // if timer > 20 minutes - // call github::update_commit_status with timeout error - // return - // wait 30 seconds - // call request_job again - // if the status is different - // call github::update_commit_status - // stop - - info!("Waiting for job to finish"); - - if now.elapsed().as_secs() == t20_minutes { - github::update_commit_status( - &github_token, - &commit_ref, - &github::CommitStatus::Error, - job_url.clone(), - Some("The status checker timed out.".to_owned()), - "continuous-integration/jenkins".to_owned() - ).expect( - format!( - "GitHub timeout error status update failed for {}/{} {}.", - commit_ref.owner, - commit_ref.repo, - commit_ref.sha - ).as_ref() - ); - - return Ok(()) - } + // Start timer + let now = Instant::now(); + + let commit_status = job.result.commit_status(); + + github::update_commit_status( + &github_token, + &commit_ref, + &commit_status, + job_url.clone(), + None, + "continuous-integration/jenkins".to_owned() + ).expect( + format!( + "GitHub pending status update failed for {}/{} {}.", + commit_ref.owner, + commit_ref.repo, + commit_ref.sha + ).as_ref() + ); + + while job.result == JobStatus::Pending { + // loop + // if timer > 20 minutes + // call github::update_commit_status with timeout error + // return + // wait 30 seconds + // call request_job again + // if the status is different + // call github::update_commit_status + // stop + + info!("Waiting for job to finish"); + + if now.elapsed().as_secs() == t20_minutes { + github::update_commit_status( + &github_token, + &commit_ref, + &github::CommitStatus::Error, + job_url.clone(), + Some("The status checker timed out.".to_owned()), + "continuous-integration/jenkins".to_owned() + ).expect( + format!( + "GitHub timeout error status update failed for {}/{} {}.", + commit_ref.owner, + commit_ref.repo, + commit_ref.sha + ).as_ref() + ); + + return Ok(()) + } - sleep(Duration::from_secs(30)); + sleep(Duration::from_secs(30)); - let updated_job = request_job( - &jenkins_url, - &jenkins_client, - job_url.as_ref() + let updated_job = request_job( + &jenkins_url, + &jenkins_client, + job_url.as_ref() + ).expect( + format!("Failed to request job '{}'.", job_url).as_ref() + ); + + if job.result != updated_job.result { + github::update_commit_status( + &github_token, + &commit_ref, + &job.result.commit_status(), + job_url.clone(), + None, + "continuous-integration/jenkins".to_owned() ).expect( - format!("Failed to request job '{}'.", job_url).as_ref() + format!( + "GitHub status update failed for {}/{} {}.", + commit_ref.owner, + commit_ref.repo, + commit_ref.sha + ).as_ref() ); - if job.result != updated_job.result { - github::update_commit_status( - &github_token, - &commit_ref, - &job.result.commit_status(), - job_url.clone(), - None, - "continuous-integration/jenkins".to_owned() - ).expect( - format!( - "GitHub status update failed for {}/{} {}.", - commit_ref.owner, - commit_ref.repo, - commit_ref.sha - ).as_ref() - ); - - return Ok(()) - } - - job = updated_job; + return Ok(()) } - // }); + + job = updated_job; + } return Ok(()) } -- cgit v1.2.3 From 01fa452209dcea101809fd1b224e029ae4f4a9d9 Mon Sep 17 00:00:00 2001 From: Teddy Wing Date: Thu, 16 Nov 2017 23:53:23 +0100 Subject: main(): Remove commented `return` This error handler now lives inside a `thread::spawn()` rather than inside the HTTP request handler, so we shouldn't return an HTTP response here. --- src/main.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) (limited to 'src') diff --git a/src/main.rs b/src/main.rs index 211a680..6bb4594 100644 --- a/src/main.rs +++ b/src/main.rs @@ -158,11 +158,7 @@ fn main() { github_token, ) { Ok(_) => {}, - Err(e) => { - error!("{}", e.to_string()); - - // return internal_server_error() - }, + Err(e) => error!("{}", e.to_string()), }; }); -- cgit v1.2.3