-
Notifications
You must be signed in to change notification settings - Fork 45
Remove assertion for connection close instead throwing StreamClosedError #291
Conversation
#290 this is same patch but compare to master. |
peer.close() | ||
peer.connections[0].closed = True | ||
|
||
tchannel.hooks.register(TestHook()) |
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.
Why don't you just call close right here before making the call instead of using the hook?
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 I call close, then we make a call, we will choose peer and rebuilt a connection.
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 meant what if you stop the server here?
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 way iostream calls the callback method is
def _run_callback(self, callback, *args):
....
with stack_context.NullContext():
self._pending_callbacks += 1
self.io_loop.add_callback(wrapper)
from the last line, we see the callback won't be called immediately, there is some delay. That's why even if I stop the server, the call back is triggered later.
for testing purpose: I manually set peer.connections[0].closed = True
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.
That's not the problem I'm trying to solve here. I'm trying to solve the hook being used to close the connection. Why not,
mock_server.tchannel._dep_tchannel.close()
That will tell the server to immediately stop listening for new incoming connections, so when the call
happens two lines below, it'll be trying to hit a hostport that doesn't exist.
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.
Also, just to confirm: Is this test just trying to verify that we raise a NetworkError
when the destination doesn't exist?
self.tchannel.event_emitter.fire( | ||
EventType.after_receive_error, req, error, | ||
) | ||
raise |
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'm a little confused here. Are you trying to re-raise the StreamClosedError
you caught or do you want the new NetworkError
to be raised?
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.
It is will be raised again since we have a try catch from the caller to tell whether we should retry based on the exception or not.
This part will be refined in this task: #285
Remove assertion for connection close instead throwing StreamClosedError
this is fix for issue: #289