-
Notifications
You must be signed in to change notification settings - Fork 0
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
cloudflare sbd-server #31
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
out.#addr = match[1] | ||
rl.close() | ||
r(out) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
throw an error here if this failed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we're ignoring a bunch of other output. We just want to resolve the promise once we get the "Ready" line.
|
||
console.log('CMD/READY') | ||
|
||
let _glb_srv = null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's a glb
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"global". We only run one server at a time. When instructed to "start" another one, we shut down the previous if any.
@@ -0,0 +1,7 @@ | |||
// This ed25519 lib annoyingly needs a hash lib explicitly configured. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could use noble-curves where you don't need to configure that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. I don't know how much I trust javascript import tree-shaking to eliminate the other curves we're not using. I guess if we wanted we could do a size comparison of the bundled output... but this works as-is currently, so I'll probably leave it alone : )
/** | ||
* Milliseconds connections are allowed to remain idle before being closed. | ||
*/ | ||
const LIMIT_IDLE_MILLIS = 10000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a timeout on the Holochain side?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sbd-client will be sent this value, and will send keep-alive messages at half this interval.
|
||
// just forward the full request / response | ||
return await stub.fetch(request); | ||
} catch (e: any) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can set useUnknownInCatchVariables: false
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is unknown
better than any
for some reason? I didn't really understand any benefit from the doc you linked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set to false
, all errors are any
. You wouldn't have to specify any
for each error explicitly.
If you set it to true
, all errors would be unknown
which means that it's no type at all and you need to use type narrowing to handle all possible errors.
|
||
await stub.forward(queue[idName]); | ||
} catch (_e: any) { | ||
// It is okay to get errors forwarding to peers, they may have |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmm... I don't know how useful logging is going to be with cloudflare. Connecting an event listener restarts all durable objects, so I don't think we're going to want to do that in production. Moreover, sending messages to peers that are now offline is a common runtime situation that would flood the logs with messages that I don't know would be useful.
I think I'm actually going to need to remove all logging rather than add any more. We may need to brainstorm other ways of debugging cloudflare workers if it comes up that we have that need.
const { pubKeyStr, pubKeyBytes } = parsePubKey(url.pathname); | ||
|
||
if (this.ctx.getWebSockets().length > 0) { | ||
throw err('websocket already connected', 400); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Errors thrown from inside this try..catch block will all be caught by the catch
and return the same status code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second param of this err
function sets a .status
property on the error that is thrown (in this case 400
) the catch
block uses this value, but defaults to 500
in case an error without a status property is thrown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I didn't look closely here.
Co-authored-by: Jost Schulte <jost.schulte@protonmail.com>
Co-authored-by: Jost Schulte <jost.schulte@protonmail.com>
No description provided.