Skip to content
This repository was archived by the owner on Jul 9, 2023. It is now read-only.

Conversation

@honfika
Copy link
Collaborator

@honfika honfika commented May 16, 2017

Doneness:

  • Build is okay - I made sure that this change is building successfully.
  • No Bugs - I made sure that this change is working properly as expected. It doesn't have any bugs that you are aware of.
  • Branching - If this is not a hotfix, I am making this request against develop branch

SessionEventArgs args, bool closeConnection)
private async Task<bool> HandleHttpSessionRequestInternal(TcpConnection connection, SessionEventArgs args, bool closeConnection)
{
bool dispose = true;
Copy link
Owner

Choose a reason for hiding this comment

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

should be disposed? I think this should be false by default and then if needs to be disposed we will set it to true. I see some inconsistency in this method.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The other methods have "disposed" flag, this is "dispose" which means that Dispose should be called. So this is not the return value, which is true when the streams were disposed.
I'll try to make it similar to the other methods, please wait for the next commit.

@honfika
Copy link
Collaborator Author

honfika commented May 16, 2017

Is this better now?

@justcoding121 justcoding121 merged commit f9ca3b0 into justcoding121:develop May 16, 2017
@justcoding121
Copy link
Owner

Thanks looks better now

justcoding121 added a commit that referenced this pull request Oct 18, 2017
move Dispose calls to finally blocks
justcoding121 added a commit that referenced this pull request Oct 18, 2017
move Dispose calls to finally blocks
justcoding121 added a commit that referenced this pull request Oct 19, 2017
move Dispose calls to finally blocks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants