-
Notifications
You must be signed in to change notification settings - Fork 42
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
chore: improve StreamManager #1994
Conversation
size-limit report 📦
|
} | ||
|
||
private prepareNewStream(peer: Peer): void { | ||
const streamPromise = this.newStream(peer).catch(() => { | ||
// No error thrown as this call is not triggered by the user | ||
const timeoutPromise = new Promise<void>((resolve) => |
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.
let's make it a util or use some small npm module for it (I prefer to have some convenient module) - we can do a follow up
return connection.newStream(this.multicodec); | ||
|
||
try { | ||
return await connection.newStream(this.multicodec); |
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.
let's rewrite to
let result;
try {
result = await ...;
}
return result;
the reason is due to huge difference in code execution because of return
or return await
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.
same for other places
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.
can you elaborate on the difference in code execution? imo both of them are the same?
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.
if try { return promise } catch ...
- it won't catch as promise is not awaited
if try { return await promise } catch ...
- will catch and the difference is only in one word
I think it is better to be on a clear side of things and write:
let result;
try {
result = await ...;
}
return result;
as the absence of await
changes so much (and I faced some nasty bugs in the past because of that)
How does this PR address the main issue? Could you share knowledge about it, please. |
My suspicion is that stream creation process and peer disconnection process are getting into a race condition where:
This PR:
|
@@ -91,7 +91,12 @@ export class FilterCore extends BaseProtocol implements IBaseProtocolCore { | |||
peer: Peer, | |||
contentTopics: ContentTopic[] | |||
): Promise<void> { | |||
const stream = await this.getStream(peer); | |||
let stream: Stream; |
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.
nit: as this pattern repeats 3 times already in the file - let's split it into private
method
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.
or because it is repeated in many other places - we can include it into .getStream
implementation
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.
include which part?
we will have to handle errors on Filter side as well
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.
I meant this code
try {
stream = await this.getStream(peer);
} catch (error) {
throw new Error(`Failed to get stream for peer ${peer.id.toString()}`);
}
```
it is ok to let `getStream` fail with message and pop up till we want to handle it, we don't need to handle it each on each level, because we are basically doing re-throw
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.
gotcha. this is now outdated code -- we now return error code instead of throwing
62ac073
to
3ec2344
Compare
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.
some open comments + validation with light-js
but overall looks good!
Problem
#1965
Solution
This PR:
Promise.race()
between successful stream creation and timeoutgetStream
to log errors when stream creation failsNotes
Contribution checklist:
!
in title if breaks public API;