Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improve treatment of SocketException #29

Merged
merged 8 commits into from
Apr 12, 2021
Merged

Improve treatment of SocketException #29

merged 8 commits into from
Apr 12, 2021

Conversation

TiagoCasas
Copy link
Contributor

@TiagoCasas TiagoCasas commented Mar 30, 2021 •
edited
Loading

Description

Exception treatment: System.Net.Sockets.SocketException

Motivation and Context

When to start Web Rest server. Everything works fine. in a few minutes, it generates the excess: Exception thrown: 'System.Net.Sockets.SocketException' in System.Net.Http.dll

Types of changes

  • Improvement (non-breaking change that improves a feature, code or algorithm)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Config and build (change in the configuration and build system, has no impact on code or features)
  • Dependencies (update dependencies and changes associated, has no impact on code or features)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@dnfadmin
Copy link

dnfadmin commented Mar 30, 2021 •
edited
Loading

CLA assistant check
All CLA requirements met.

@josesimoes
Copy link
Member

@Ellerbach this try/catch it's better placed on the Http ReceiveLine or caller, isn't it?

@Ellerbach
Copy link
Member

I think both. Reason is something can go wrong at any place in the main WebServer loop. If it goes wrong in the user call, for some reasons, the server will go down.
Now, yes, it's maybe not on the all loop that it should be placed, that's open for debate.
On the http listener side, we need a catch as well. As this will be outside of this try/catch loop. The exception on the listenr will be raised here and not catch: HttpListenerContext context = _listener.GetContext();

@josesimoes
Copy link
Member

As we've briefly talked on Discord, I suggest to add a global try / catch in InputNetworkStreamWrapper.Read_HTTP_Line().
@TiagoCasas can you handle this please?

As for the changes introduced here @Ellerbach not sure how you want to proceed...

@TiagoCasas
Copy link
Contributor Author

TiagoCasas commented Apr 2, 2021 via email

@josesimoes josesimoes requested review from Ellerbach and removed request for Ellerbach April 5, 2021 09:50
Ellerbach
Ellerbach reviewed Apr 9, 2021
View reviewed changes
Copy link
Member

@Ellerbach Ellerbach left a comment

Choose a reason for hiding this comment

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

looks good to me but why di you moved the declared variables in the foreach loop?

@TiagoCasas
Copy link
Contributor Author

TiagoCasas commented Apr 9, 2021 via email

Ellerbach
Ellerbach reviewed Apr 10, 2021
View reviewed changes
Copy link
Member

@Ellerbach Ellerbach left a comment

Choose a reason for hiding this comment

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

Thanks for this new version. Some comments to address for performance. I think you brought clarity in the code all up. Thanks for this.

nanoFramework.WebServer/WebServer.cs
string toCompare = route.CaseSensitive ? rawUrl : rawUrl.ToLower();

if (urlParam > 0)
isFound = urlParam == routeStr.Length + incForSlash;
Copy link
Member

Choose a reason for hiding this comment

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

please add { } even for 1 line if / else. This avoid a lot of issues?

nanoFramework.WebServer/WebServer.cs
&& (route.Method == string.Empty || context.Request.HttpMethod == route.Method);

if (!neededName || !neededName2)
continue;
Copy link
Member

Choose a reason for hiding this comment

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

Even for 1 line, please add the { }

nanoFramework.WebServer/WebServer.cs Outdated
if (toCompare.IndexOf(routeStr) == incForSlash)
CallbackRoutes route = (CallbackRoutes)rt;

bool isFound;
Copy link
Member

Choose a reason for hiding this comment

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

Reason why isFoud and all the others are declared outside of the foreach loop is for performance reasons with nanoFramework. In this loop, it will be allocated and reallocated and again and again. So allocating it as high as possible will save from this behavior and optimize the speed of execution.
So please move them back where they were. If you think what they do miss clarity, feel free to add a comment :-)

nanoFramework.WebServer/WebServer.cs Outdated
else
isFound = toCompare.Length == routeStr.Length + incForSlash;

// todo - please help to name the variables: neededName, neededName2
Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with the neededName. For the seconf one neededMethod can be good. And maybe what's missing is just a comment?
something like:
// neededName = matching the route name
// neededMethod = matching the method type

nanoFramework.WebServer/WebServer.cs
isFound = urlParam == routeStr.Length + incForSlash;
else
isFound = toCompare.Length == routeStr.Length + incForSlash;

Copy link
Member

Choose a reason for hiding this comment

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

if not found, we can continue at this point. That will improve the performance.

Ellerbach
Ellerbach approved these changes Apr 11, 2021
View reviewed changes
Copy link
Member

@Ellerbach Ellerbach left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks!

@josesimoes josesimoes changed the title Exception treatment: System.Net.Sockets.SocketException Improve treatment of SocketException Apr 12, 2021
@josesimoes josesimoes merged commit efafd21 into nanoframework:develop Apr 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ellerbach Ellerbach Ellerbach approved these changes

Assignees
No one assigned
Labels
Type: enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@TiagoCasas @dnfadmin @josesimoes @Ellerbach @nfbot