-
Notifications
You must be signed in to change notification settings - Fork 5
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.
I am not certain this fixes the issue ... I feel like this just pushes under the rug potential cases wher EOF
was an error.
Arguably the bigger problem is that we do log this error so I feel like we should just not log it 🤷 and just emit an error event 🤷
Maybe if there is no error event - log it there?
Co-authored-by: Mihail Stoykov <[email protected]>
I think we're safe here, because of that. |
8b18ee9
to
f417419
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.
LGTM in general.
But I feel like we should not be emitting even the EOF as we can't be certain it is not an actual error that should be reported to the user. If the user thinks that - they can ignore it IMO.
Maybe this should be discussed with the reporter 🤔
if errors.Is(err, io.EOF) { | ||
s.logger.WithError(err).Debug("skip sending a message stream is cancelled/finished") | ||
err = nil | ||
} |
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 would still prefer to just skip that ...
We can't know if this EOF is because that was what was expected.
If it isn't this is just shuffing it under the rug.
If it is - I feel the user should fix that. Maybe we can not emit it if the WebSocket (js object) is already closed, but doesn't seem like what the user reported to me.
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 it isn't this is just shuffing it under the rug.
But according to to this, the real error will be returned with the RecvMsg
. So EOF in sending context, is not the real error.
However, I now I'm thinking that we should not do closeWithError(nil)
for that case 🤔 since it could hijack the real error reporting. 🤔
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.
However, I now I'm thinking that we should not do closeWithError(nil)
In principle, it sounds correct to me
since it could hijack the real error reporting
but I'm not getting how this is possible. It sounds like closeWithError
does not affect directly the read loop which is the place where it is expected to get the real error as you reported.
Or is it for the fact that closeWithError is called twice but the second is discarded?
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 is it for the fact that closeWithError is called twice but the second is discarded?
yeah, at the current state, closeWithError
is designed to discard the second error if the stream is closed. So I think that I need to re-think this and maybe split the responsibilities to actually about the closing and error reporting.
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.
So I think that I need to re-think this and maybe split the responsibilities to actually about the closing and error reporting.
What is the expectation, will it land in this PR?
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.
Hey @codebien, there are a couple of actions that I'm going to do, and they are on my list. No worries and no need to push this now.
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.
Okay, I've pushed the commit where tried to address the issue that I've described above:
However, I now I'm thinking that we should not do closeWithError(nil) for that case thinking since it could hijack the real error reporting. thinking
Regarding the omitting the EOF (in sending), I still believe that's the right thing since as I linked to the grpc's code the real error if that happens is routing to the RecvMsg
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.
Marking this as a request changes so you can re-request a new review when you want a new one.
f417419
to
a3f8a24
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.
LGTM
s.close(err) | ||
|
||
return s.callErrorListeners(err) |
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.
This means that a user might get multiple errors to be handled. Not certain if that is good or bad ...but I guess we can try it.
+ always try to trigger error listeners
a3f8a24
to
5b3c8bc
Compare
What?
In case some message was in fly and stream close, we could produce the error that was reported in the #18
Just in case, it's just aligning with the existing thing that we have in the write method, where we discard messages if a stream is closed/ing.
Why?
Closes: #18