Skip to content

Adds reliability and simplifies, finalizes v1 HTTP API#6

Merged
johnnyman727 merged 1 commit intomasterfrom
jon-1.0.0
Jul 13, 2016
Merged

Adds reliability and simplifies, finalizes v1 HTTP API#6
johnnyman727 merged 1 commit intomasterfrom
jon-1.0.0

Conversation

@johnnyman727
Copy link
Contributor

Previously, the HTTP API required headers to indicate the name of the compiled binary and the name of the project folder. This PR removes those headers and instead deduces the name of the project after unpacking it and grabs the binary name from the Cargo.toml.

Additionally, this PR adds JSON support to the response which not only makes the error handling easier for the client but allows the server to send down compilation logs.

This server should be all set for 1.0.0 release.

@johnnyman727 johnnyman727 changed the title Adds reliability and simplifies HTTP API Adds reliability and simplifies, finalizes v1 HTTP API Jul 1, 2016
index.js Outdated
var cargoToml = toml.parse(fs.readFileSync(path.join(projectPath, 'Cargo.toml'), 'utf8'));
}
catch (error) {
debug("Problem parsing Cargo.toml:", error);
Copy link

Choose a reason for hiding this comment

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

Nit: space missing at end of Cargo.toml

@johnnyman727 johnnyman727 force-pushed the jon-1.0.0 branch 2 times, most recently from 14dfcaa to 61d50dc Compare July 2, 2016 15:26
@johnnyman727
Copy link
Contributor Author

@cflewis updated with your comments.

@cflewis
Copy link

cflewis commented Jul 2, 2016

LGTM

@johnnyman727
Copy link
Contributor Author

Thanks! We'll merge whenever we merge the CLI branch just in case there are some more changes to be made.

// When the binary has been written,
packingStream.once('finish', () => {
// Write the ending quote and bracket for the JSON
res.end(`"}`);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm in the process of deploying this Dockerfile to Digital Ocean. Unfortunately, during testing, I find that the server fails at the point of sending the binary back down to the client with:

  server output of compilation - error: null, stdout:    Compiling hello v0.1.0 (file:///tmp/tessel-rust-compile116613-271-qr3udd/hello)
, stderr:  +495ms
  server Finished cross compilation job. +533ms
events.js:141
      throw er; // Unhandled 'error' event
      ^

Error: write after end
    at ServerResponse.OutgoingMessage.write (_http_outgoing.js:430:15)
    at Base64Encode.ondata (_stream_readable.js:542:20)
    at emitOne (events.js:77:13)
    at Base64Encode.emit (events.js:169:7)
    at Base64Encode.Readable.read (_stream_readable.js:368:10)
    at flow (_stream_readable.js:759:26)
    at ServerResponse.<anonymous> (_stream_readable.js:616:7)
    at emitNone (events.js:67:13)
    at ServerResponse.emit (events.js:166:7)
    at Socket.ondrain (_http_common.js:215:44)

It looks to me like the packingStream.once('finish') event is being fired before the packingStream actually stops finishes writing to the res. Then, we write the final bracket to res, packingStream tries to write to it again, and the error is thrown. @LinusU any ideas why finish isn't being thrown at the proper time? It's also interesting that it works fine locally but fails when deployed remotely to Digital Ocean.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might also be helpful to know that if I replace packingStream.once('finish', ... with a setTimeout(..., 2000), it completes successfully. This confirms to me that the finish event is being fired too early.

Copy link
Contributor Author

@johnnyman727 johnnyman727 Jul 13, 2016

Choose a reason for hiding this comment

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

It might also be helpful to note that this hack seems to un-break it:

var tarStream = tar.pack(binaryPath, {entries: [binaryName]});
var encodeStream = base64.encode();

tarStream.on('data', (d) => encodeStream.write(d));

tarStream.once('end', () => encodeStream.end());

encodeStream.on('data', (d) => res.write(d));

encodeStream.once('finish', () => {
   res.write("}");
});

Copy link

Choose a reason for hiding this comment

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

I think the problem is that the finish event if for a writeable stream to indicate that all data has been written to it (i.e. it has been ended). My guess to way this happened to work locally for you is that it will actually work about 25% of the times because of how base64 works (see here).

I think that the fix is as easy as changing .once('finish') into .once('end'). 👍

The end is event is a readable stream event that says that the stream has ended. Since this is a duplex stream it will emit both the writeable events and the readable events.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 confirmed fix!

@johnnyman727
Copy link
Contributor Author

This was deployed on Digital Ocean with:

docker pull johnnyman727/rust-compilation-server:v1
docker run -p 49160:8080 johnnyman727/rust-compilation-server:v1

@Frijol Frijol deleted the jon-1.0.0 branch July 13, 2016 18:54
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.

4 participants