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

Smithy4s integration #58

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Smithy4s integration #58

wants to merge 7 commits into from

Conversation

Baccata
Copy link
Contributor

@Baccata Baccata commented Feb 21, 2023

Adds a smithy4s module, allowing to automatically derive endpoints and stubs from smithy definitions

Todos :

  • Find better names ?
  • Unit tests
  • Documentation

@Baccata
Copy link
Contributor Author

Baccata commented Feb 21, 2023

@armanbilge, good progress so far.

The examples currently hang at the end, but otherwise work as expected : there are two example applications involved, a client and a server. The build is wired in such a way that the client receives through the forkEnv the path to an assembly jar of the server when running mill -i examples.smithyClient.run. The client starts the server in a subprocess, wires the various streams and pipes in the relevant manner, and start talking to the server.

A test.smithy file defines the interfaces of both parties. The code is generated, and the new ClientStub and ServerEndpoints (temporary names) constructs are used to get the smithy4s code work with the jsonrpclib code.

Regarding the hanging : I presume the ChildProcess I've copied and adapted from Davenverse is not sound. I wonder whether it's a similar thing to what led you to run fs2.io.stdin on a dedicated pool ... I could use some help to understand how to troubleshoot it.

I mean, it's entirely possible/likely that I've messed up something else, but that subprocess stdin/stdout based communication is the only I/O code involved in the example ...

@@ -0,0 +1,23 @@
$version: "2.0"

namespace jsonrpclib
Copy link
Contributor Author

@Baccata Baccata Feb 21, 2023

Choose a reason for hiding this comment

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

this smithy file contains the protocol definition. It kind of echoes a question that @ckipp01 had on my BSP PR about where would be a good "central" for protocols.

I suppose DisneyStreaming could host it in https://github.com/disneystreaming/alloy/, but I'm a bit wary about putting it there and have the BSP depend on it ($work project etc etc).

import scala.jdk.CollectionConverters._
import java.io.OutputStream

trait ChildProcess[F[_]] {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

looking forward to killing this off when fs2.io has a solution for it

Comment on lines +34 to +41
def stdout: fs2.Stream[F, Byte] = fs2.io
.readInputStream[F](Sync[F].interruptible(p.getInputStream()), chunkSize = readBufferSize)

def stderr: fs2.Stream[F, Byte] = fs2.io
.readInputStream[F](Sync[F].blocking(p.getErrorStream()), chunkSize = readBufferSize)
// Avoids broken pipe - we cut off when the program ends.
// Users can decide what to do with the error logs using the exitCode value
.interruptWhen(done.void.attempt)

Choose a reason for hiding this comment

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

readInputStream is not using interruptible looks like.
https://github.com/typelevel/fs2/blob/56b86022d48d3caf0a9b5f00bf637cac62bb7f5e/io/shared/src/main/scala/fs2/io/io.scala#L46

So that would probably explain why it is hanging. We can try swapping that, but as you mentioned interruptibility might be broken like it is for stdin. In which case I suppose each Process would have to start its own dedicated blocking threads that it can kill 🤔

Choose a reason for hiding this comment

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

I published 3.7-ae0fc02-SNAPSHOT which swapped in interruptible, you can give that a try and see if it helps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't resolve yet, are you sure it's the right version ?

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doh, my bad. Will try in a bit

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mmm I only get the initial prints from both the client and server side, but then nothing ... It may be because chunks are not automatically flushed in your implementation. That's why I couldn't use the Davenverse lib directly :

https://github.com/neandertech/jsonrpclib/pull/58/files#diff-281c0b146d38f8e1fc9f7c77eb87773debe12d364454ef6766472a26563a0ee6R47-R59

Choose a reason for hiding this comment

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

Woops, I missed that. Thanks!

Choose a reason for hiding this comment

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

Ok, try this one 3.7-eacce62-SNAPSHOT.

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 works 🎉 most things that should print actually print (except for the server termination message but that's some userland problem), and the program terminates as expected !

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.

2 participants