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

Implement block on connect with Rust+Node.js SDK #953

Merged
merged 1 commit into from
Jul 30, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions build/includes/sdk.mk
Original file line number Diff line number Diff line change
Expand Up @@ -122,15 +122,17 @@ run-sdk-conformance-local: ensure-agones-sdk-image
-e "TEST=$(TESTS)" -e "TIMEOUT=$(TIMEOUT)" $(sidecar_tag)

# Run SDK conformance test, previously built, for a specific SDK_FOLDER
# Sleeps the start of the sidecar to test that the SDK blocks on connection correctly
run-sdk-conformance-no-build: TIMEOUT ?= 30
run-sdk-conformance-no-build: RANDOM := $(shell bash -c 'echo $$RANDOM')
run-sdk-conformance-no-build: DELAY ?= $(shell bash -c "echo $$[ ($(RANDOM) % 5 ) + 1 ]s")
run-sdk-conformance-no-build: TESTS ?= ready,allocate,setlabel,setannotation,gameserver,health,shutdown,watch
run-sdk-conformance-no-build: ensure-agones-sdk-image
run-sdk-conformance-no-build: ensure-build-sdk-image
sleep 2s && DOCKER_RUN_ARGS="--network=host $(DOCKER_RUN_ARGS)" COMMAND=sdktest $(MAKE) run-sdk-command & \
docker run -p 59357:59357 -e "ADDRESS=" \
DOCKER_RUN_ARGS="--network=host $(DOCKER_RUN_ARGS)" COMMAND=sdktest $(MAKE) run-sdk-command & \
sleep $(DELAY) && docker run -p 59357:59357 -e "ADDRESS=" \
Copy link
Member Author

Choose a reason for hiding this comment

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

@aLekSer wdyt of this approach?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, this one much better

-e "TEST=$(TESTS)" -e "TIMEOUT=$(TIMEOUT)" --net=host $(sidecar_tag)


# Run SDK conformance test for a specific SDK_FOLDER
run-sdk-conformance-test:
$(MAKE) run-sdk-command COMMAND=build-sdk-test
Expand Down
25 changes: 25 additions & 0 deletions sdks/nodejs/spec/agonesSDK.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,31 @@ describe('agones', () => {
agonesSDK = new AgonesSDK();
});

describe('connect', () => {
it('calls the server and handles success', async () => {
spyOn(agonesSDK.client, 'waitForReady').and.callFake((deadline, callback) => {
let result = new messages.Empty();
callback(undefined, result);
});
let result = await agonesSDK.connect();
expect(agonesSDK.client.waitForReady).toHaveBeenCalled();
expect(result).toEqual(undefined);
});

it('calls the server and handles failure', async () => {
spyOn(agonesSDK.client, 'waitForReady').and.callFake((deadline, callback) => {
callback('error', undefined);
});
try {
await agonesSDK.connect();
fail();
} catch (error) {
expect(agonesSDK.client.waitForReady).toHaveBeenCalled();
expect(error).toEqual('error');
}
});
});

describe('allocate', () => {
it('calls the server and handles success', async () => {
spyOn(agonesSDK.client, 'allocate').and.callFake((request, callback) => {
Expand Down
12 changes: 12 additions & 0 deletions sdks/nodejs/src/agonesSDK.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,18 @@ class AgonesSDK {
this.emitters = [];
}

async connect() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should add a test for this to maintain code coverage. Please let me know if you'd like me to add, e.g. separate PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure how we are going to make a unit test for this, when there is nothing to connect to, but I will have a look, see if I can work out a way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Aha, got it - there is a mocking library you use to mock out the client. On it!

Copy link
Member Author

Choose a reason for hiding this comment

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

Done! PTAL.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should also update the docs.
I will look at generating API docs automatically for Node.js soon (maybe next weekend)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep - on my todo list.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!. PTAL.

return new Promise((resolve, reject) => {
this.client.waitForReady(Date.now() + 30000, (error) => {
if (error) {
reject(error);
} else {
resolve();
}
})
});
}

async close() {
if (this.healthStream !== undefined) {
this.healthStream.destroy();
Expand Down
34 changes: 26 additions & 8 deletions sdks/rust/src/sdk.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,18 @@
// See the License for the specific language governing permissions and
// limitations under the License.

use futures::{Future, Sink, Stream};
use grpcio;
use std::sync::{Arc, Mutex};
use std::thread::sleep;
use std::time::Duration;

use futures::{Future, Sink, Stream};
use grpcio;
use grpcio::CallOption;
use protobuf::Message;

use errors::*;
use grpc::sdk;
use grpc::sdk_grpc;
use protobuf::Message;
use types::*;

const PORT: i32 = 59357;
Expand All @@ -34,7 +37,7 @@ pub struct Sdk {
impl Sdk {
/// Starts a new SDK instance, and connects to localhost on port 59357.
/// Blocks until connection and handshake are made.
/// Times out after 30 seconds.
/// Times out after ~30 seconds.
pub fn new() -> Result<Sdk> {
let addr = format!("localhost:{}", PORT);
let env = Arc::new(grpcio::EnvBuilder::new().build());
Expand All @@ -43,7 +46,24 @@ impl Sdk {
.connect(&addr);
let cli = sdk_grpc::SdkClient::new(ch);
let req = sdk::Empty::new();
let _ = cli.ready(&req).map(Box::new)?;

Copy link
Member Author

Choose a reason for hiding this comment

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

@thara would love your review of my Rust code. I'm a little.... rusty 🙄 🤣 🦀

// Unfortunately there isn't a native way to block until connected
// so we had to roll our own.
let mut counter = 0;
loop {
counter += 1;
match cli.get_game_server(&req) {
Ok(_) => break,
Err(e) => {
if counter > 30 {
return Err(ErrorKind::Grpc(e).into());
}
sleep(Duration::from_secs(1));
continue;
}
}
}

let (sender, _) = cli.health()?;
Ok(Sdk {
client: Arc::new(cli),
Expand All @@ -58,15 +78,13 @@ impl Sdk {
Ok(res)
}


/// Allocate the Game Server
/// Allocate the Game Server
pub fn allocate(&self) -> Result<()> {
let req = sdk::Empty::default_instance();
let res = self.client.allocate(req).map(|_| ())?;
Ok(res)
}


/// Marks the Game Server as ready to shutdown
pub fn shutdown(&self) -> Result<()> {
let req = sdk::Empty::default_instance();
Expand Down
12 changes: 11 additions & 1 deletion site/content/en/docs/Guides/Client SDKs/nodejs.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,24 @@ Add the agones dependency to `package.json`, replacing with the download locatio
}
```

To begin working with the SDK, create an instance of it. This will open a connection to the SDK server.
To begin working with the SDK, create an instance of it. {{< feature expiryVersion="0.12.0" >}}This will open a connection to the SDK server.{{< /feature >}}

```javascript
const AgonesSDK = require('agones');

let agonesSDK = new AgonesSDK();
```

{{% feature publishVersion="0.12.0" %}}

To connect to the SDK server, either local or when running on Agones, run the `async` method `sdk.connect()`, which will
`resolve` once connected or `reject` on error or if no connection can be made after 30 seconds.

```javascript
await agonesSDK.connect();
```
{{% /feature %}}

To send a [health check]({{< relref "_index.md#health" >}}) ping call `health()`.

```javascript
Expand Down
9 changes: 9 additions & 0 deletions site/content/en/docs/Guides/Client SDKs/rust.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,15 @@ if sdk.health().is_ok() {
}
```

{{% feature publishVersion="0.12.0" %}}

To mark the [game session as ready]({{< relref "_index.md#ready" >}}) call `sdk.ready()`.

```rust
sdk.ready()?;
```
{{% /feature %}}

To mark that the [game session is completed]({{< relref "_index.md#shutdown" >}}) and the game server should be shut down call `sdk.shutdown()`.

```rust
Expand Down
3 changes: 3 additions & 0 deletions test/sdk/nodejs/testSDKClient.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ const agonesSDK = new AgonesSDK();
const connect = async () => {
let UID = '';
try {
console.log("attempting to connect");
await agonesSDK.connect();
console.log("connected!");
let once = true;
agonesSDK.watchGameServer((result) => {
console.log('watch', result);
Expand Down
16 changes: 16 additions & 0 deletions test/sdk/rust/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# Copyright 2019 Google LLC All Rights Reserved.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

/target/
Cargo.lock
2 changes: 1 addition & 1 deletion test/sdk/rust/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ fn run() -> Result<(), String> {
thread::sleep(Duration::from_secs(5));
}

println!("Shutting down after 20 seconds...");
println!("Shutting down...");
sdk.shutdown()
.map_err(|e| format!("Could not run Shutdown: {}. Exiting!", e))?;
println!("...marked for Shutdown");
Expand Down