Exception handling & memory leak fixes#150
Conversation
|
|
josesimoes
left a comment
josesimoes
left a comment
There was a problem hiding this comment.
Please read my comments on the various points.
Also do add verbose comments explaining the reasoning and why something is being done, but remove the dates and your github user name from the comment.
All changes will be logged in the file history and credited to the dev that has done them, no need to clutter the code with distracting text.
All in all, great job catching all these! 👍🏻
| m_DropOldConnectionsTimer.Change( HttpConstants.DefaultKeepAliveMilliseconds, System.Threading.Timeout.Infinite ); | ||
| } | ||
| } | ||
| } catch { |
There was a problem hiding this comment.
Need to add a comment here on why this is catching all and why there is nothing to deal with.
There was a problem hiding this comment.
nanoFramework applications were crashing due to unhandled exceptions.
This entire section of code will eventually need to be re-worked to support persistent connections.
Some exceptions will likely require more specific action here.
There was a problem hiding this comment.
I think @josesimoes meant adding a comment in the code 😉
There was a problem hiding this comment.
try/catch block removed & comments added/corrected throughout
| InputNetworkStreamWrapper streamWrapper = (InputNetworkStreamWrapper)m_ConnectedStreams[i]; | ||
| TimeSpan timePassed = curTime - streamWrapper.m_lastUsed; // skigrinder - this is correct - original code was (m_lastUsed - curTime) - good evidence that persistent connections were never implemented here | ||
| // If the socket is old, then close and remove from the list. | ||
| if (timePassed.TotalMilliseconds > HttpConstants.DefaultKeepAliveMilliseconds) // skigrinder - original code used timePassed.Milliseconds - need to use TotalMilliseconds |
There was a problem hiding this comment.
Thanks. Are persistent connections a big priority?
There was a problem hiding this comment.
Don't recall these been asked before... so the answer is no, not a big priority. But if you can have those work, that would be a nice improvement.
There was a problem hiding this comment.
I'd eventually like to take a run at persistent connections.
This would probably improve http send/receive performance.
Can we commit/merge the existing fixes first?
josesimoes
left a comment
josesimoes
left a comment
There was a problem hiding this comment.
OK to merge. Just waiting for a fix on the build pipeline for the final check.
5 participants
Description
Modifications to Net and Net.Http libraries to fix exception handling & memory leak issues that were causing nanoFramework applications to crash
Motivation and Context
nanoFramework applications crashed when attempting to send http requests between ESP32 units.
A memory leak issue was causing nanoFramework applications to crash on ESP32 units.
No nanoFramework issues were opened for these problems
How Has This Been Tested?
Modifications were tested with multiple ESP32 units.
Http requests can now be sent/received without crashing the ESP32.
Verified that the memory leak issue associated with this library has also been fixed.
Screenshots
Types of changes
Checklist: