Skip to content

Exception handling & memory leak fixes #150

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

Merged
josesimoes merged 11 commits into from
Dec 29, 2020
Merged

Exception handling & memory leak fixes #150

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
8a99ac7
Exception handling & memory leak fixes
skigrinder Dec 11, 2020
de5680c
Code & comment cleanup as per Jose's comments
skigrinder Dec 17, 2020
ff35201
Removed try/catch blocks to allow normal .NET exception propagation.
skigrinder Dec 20, 2020
dea00a7
Mostly code style fixes
josesimoes Dec 22, 2020
0e3a93b
Merge branch 'develop' into skigrinder---Exception-handling-&-memory-…
josesimoes Dec 22, 2020
54f4bcb
Merge branch 'develop' into skigrinder---Exception-handling-&-memory-…
josesimoes Dec 22, 2020
5ff91f8
Update System.Net._OutputNetworkStreamWrapper.cs
josesimoes Dec 27, 2020
01ff6eb
Update System.Net.HttpWebRequest.cs
josesimoes Dec 28, 2020
b6adf9e
Remove un-necessary whitespace
networkfusion Dec 28, 2020
f83bea5
Improve comment.
networkfusion Dec 28, 2020
a4bd54f
Merge branch 'develop' into skigrinder---Exception-handling-&-memory-…
josesimoes Dec 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 51 additions & 24 deletions nanoFramework.System.Net.Http/Http/System.Net.HttpWebRequest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,36 +73,40 @@ public class HttpWebRequest : WebRequest
/// <param name="unused">Unused</param>
static private void CheckPersistentConnections(object unused)
{
// Persistent connections have not been properly implemented yet.
int count = m_ConnectedStreams.Count;

// The fastest way to exit out - if there are no sockets in the list - exit out.
if (count > 0)
{
DateTime curTime = DateTime.UtcNow;

lock (m_ConnectedStreams)
{
count = m_ConnectedStreams.Count;

for (int i = count-1; i >= 0; i--)
for (int i = count - 1; i >= 0; i--)
{
InputNetworkStreamWrapper streamWrapper = (InputNetworkStreamWrapper)m_ConnectedStreams[i];

TimeSpan timePassed = streamWrapper.m_lastUsed - curTime;
TimeSpan timePassed = curTime - streamWrapper.m_lastUsed;

// If the socket is old, then close and remove from the list.
if (timePassed.Milliseconds > HttpConstants.DefaultKeepAliveMilliseconds)
if (timePassed.TotalMilliseconds > HttpConstants.DefaultKeepAliveMilliseconds)
{
m_ConnectedStreams.RemoveAt(i);

// Closes the socket to release resources.
streamWrapper.Dispose();
}
}

// turn off the timer if there are no active streams
if(m_ConnectedStreams.Count > 0)
if (m_ConnectedStreams.Count > 0)
{
m_DropOldConnectionsTimer.Change( HttpConstants.DefaultKeepAliveMilliseconds, System.Threading.Timeout.Infinite );
m_DropOldConnectionsTimer.Change(HttpConstants.DefaultKeepAliveMilliseconds, System.Threading.Timeout.Infinite);
}

}
}
}
Expand Down Expand Up @@ -1336,8 +1340,8 @@ private InputNetworkStreamWrapper EstablishConnection(Uri proxyServer, Uri targe
// But first we need to know that socket is not closed.
try
{
// If socket is closed ( from this or other side ) the call throws exception.
if (inputStream.m_Socket.Poll(1, SelectMode.SelectWrite))
// If socket is closed (from this or other side) the call throws exception.
if (inputStream.m_Socket.Poll(-1, SelectMode.SelectWrite))
{
// No exception, good we can condtinue and re-use connected stream.

Expand All @@ -1352,7 +1356,6 @@ private InputNetworkStreamWrapper EstablishConnection(Uri proxyServer, Uri targe
{
removeStreamList.Add(inputStream);
}

}
catch (Exception)
{
Expand Down Expand Up @@ -1415,13 +1418,15 @@ private InputNetworkStreamWrapper EstablishConnection(Uri proxyServer, Uri targe
}

// If socket was not found in waiting connections, then we create new one.
Socket socket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
Socket socket = null;
socket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);

try
Copy link
Copy Markdown
Member

@josesimoes josesimoes Dec 17, 2020

Choose a reason for hiding this comment

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

I see where you're getting with this and it makes sense to wrap the call to create a new socket.
Suggest that you add the code inside the if ahead to the catch block and add a comment explaining why we this is a catch all and there is no way to get out of it nicely so we better quit here.

Copy link
Copy Markdown
Contributor Author

@skigrinder skigrinder Dec 24, 2020

Choose a reason for hiding this comment

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

try/catch block removed to allow normal .NET exception handling.

{
socket.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.ReuseAddress, true);
}
catch{}

try
{
socket.SetSocketOption(SocketOptionLevel.Tcp, SocketOptionName.NoDelay, true);
Expand All @@ -1436,6 +1441,9 @@ private InputNetworkStreamWrapper EstablishConnection(Uri proxyServer, Uri targe
}
catch (SocketException e)
{
// need to close socket, otherwise this will cause an out of memory exception
socket.Close();

throw new WebException("connection failed", e, WebExceptionStatus.ConnectFailure, null);
}

Expand Down Expand Up @@ -1476,14 +1484,18 @@ private InputNetworkStreamWrapper EstablishConnection(Uri proxyServer, Uri targe
retStream.m_rmAddrAndPort = m_originalUrl.Host + ":" + m_originalUrl.Port;
}

lock (m_ConnectedStreams)
// Check keepAlive before creating persistent connection - persistent connections are not currently supported
if (m_keepAlive)
{
m_ConnectedStreams.Add(retStream);

// if the current stream list is empty then start the timer that drops unused connections.
if (m_ConnectedStreams.Count == 1)
lock (m_ConnectedStreams)
{
m_DropOldConnectionsTimer.Change(HttpConstants.DefaultKeepAliveMilliseconds, System.Threading.Timeout.Infinite);
m_ConnectedStreams.Add(retStream);

// if the current stream list is empty then start the timer that drops unused connections.
if (m_ConnectedStreams.Count == 1)
{
m_DropOldConnectionsTimer.Change(HttpConstants.DefaultKeepAliveMilliseconds, System.Threading.Timeout.Infinite);
}
}
}
}
Expand All @@ -1499,14 +1511,16 @@ private void SubmitRequest()
// We have connected socket. Create request stream
// If proxy is set - connect to proxy server.

if(m_requestStream == null)
if (m_requestStream == null)
{
if (m_proxy == null)
{ // Direct connection to target server.
{
// Direct connection to target server.
m_requestStream = EstablishConnection(m_originalUrl, m_originalUrl);
}
else // Connection through proxy. We create network stream connected to proxy
else
{
// Connection through proxy. We create network stream connected to proxy
Uri proxyUri = m_proxy.GetProxy(m_originalUrl);

if (m_originalUrl.Scheme == "https")
Expand All @@ -1522,6 +1536,13 @@ private void SubmitRequest()
}
}

if (m_requestStream == null)
{
// Connection could not be established
m_requestSent = false;
return;
}

// We have connected stream. Set the timeout from HttpWebRequest
m_requestStream.WriteTimeout = m_readWriteTimeout;
m_requestStream.ReadTimeout = m_readWriteTimeout;
Expand Down Expand Up @@ -1725,6 +1746,12 @@ public override WebResponse GetResponse()
SubmitRequest();
}

// Need to check m_requestSent after SubmitRequest()
if (!m_requestSent)
Comment thread
josesimoes marked this conversation as resolved.
{
return response;
}

CoreResponseData respData = null;

// reset the total response bytes for the new request.
Expand Down Expand Up @@ -1765,8 +1792,9 @@ public override WebResponse GetResponse()
m_responseStatus = response.StatusCode;

m_responseCreated = true;
m_requestStream.m_InUse = false; // Persistent connections are not yet supported, but they wouldn't work without this.
}
catch(SocketException se)
catch (SocketException se)
{
if (m_requestStream != null)
{
Expand All @@ -1777,12 +1805,11 @@ public override WebResponse GetResponse()
this.m_requestStream.m_Socket.Close();
}
}

throw new WebException("", se);
throw new WebException("GetResponse() failed", se);
}
catch(Exception e)
catch (Exception e)
{
throw new WebException("", e);
throw new WebException("GetResponse() failed", e);
}


Expand Down
5 changes: 3 additions & 2 deletions nanoFramework.System.Net.Http/Http/System.Net._OutputNetworkStreamWrapper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,8 +144,9 @@ public override void Flush()
// Calls HttpListenerResponse.SendHeaders. HttpListenerResponse.SendHeaders sets m_headersSend to null.
m_headersSend();
}

m_Stream.Flush();

// Need to check for null before using here
m_Stream?.Flush();
}

/// <summary>
Expand Down