From 8a99ac726313ef8ce77556274766f162d2b31de3 Mon Sep 17 00:00:00 2001 From: skigrinder <62192313+skigrinder@users.noreply.github.com> Date: Thu, 10 Dec 2020 20:35:57 -0700 Subject: [PATCH 1/8] Exception handling & memory leak fixes --- .../Http/System.Net.HttpWebRequest.cs | 192 +++++++++++------- .../System.Net._OutputNetworkStreamWrapper.cs | 19 +- 2 files changed, 137 insertions(+), 74 deletions(-) diff --git a/nanoFramework.System.Net.Http/Http/System.Net.HttpWebRequest.cs b/nanoFramework.System.Net.Http/Http/System.Net.HttpWebRequest.cs index 46db3e94..f2824999 100644 --- a/nanoFramework.System.Net.Http/Http/System.Net.HttpWebRequest.cs +++ b/nanoFramework.System.Net.Http/Http/System.Net.HttpWebRequest.cs @@ -73,38 +73,47 @@ public class HttpWebRequest : WebRequest /// Unused static private void CheckPersistentConnections(object unused) { - int count = m_ConnectedStreams.Count; - // The fastest way to exit out - if there are no sockets in the list - exit out. - if (count > 0) + // skigrinder - 2020-12-10 added exception handling & fixed 'timePassed' setting + try { - DateTime curTime = DateTime.UtcNow; - lock (m_ConnectedStreams) + int count = m_ConnectedStreams.Count; + // The fastest way to exit out - if there are no sockets in the list - exit out. + if (count > 0) { - count = m_ConnectedStreams.Count; - - for (int i = count-1; i >= 0; i--) + DateTime curTime = DateTime.UtcNow; + lock (m_ConnectedStreams) { - InputNetworkStreamWrapper streamWrapper = (InputNetworkStreamWrapper)m_ConnectedStreams[i]; - - TimeSpan timePassed = streamWrapper.m_lastUsed - curTime; - - // If the socket is old, then close and remove from the list. - if (timePassed.Milliseconds > HttpConstants.DefaultKeepAliveMilliseconds) + count = m_ConnectedStreams.Count; + for (int i = count - 1; i >= 0; i--) { - m_ConnectedStreams.RemoveAt(i); - - // Closes the socket to release resources. - streamWrapper.Dispose(); + try + { + 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 + // - good indication that persistent connections was never implemented in this library + { + 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) + { + m_DropOldConnectionsTimer.Change(HttpConstants.DefaultKeepAliveMilliseconds, System.Threading.Timeout.Infinite); + } + } + catch + { + m_ConnectedStreams.RemoveAt(i); + } } } - - // turn off the timer if there are no active streams - if(m_ConnectedStreams.Count > 0) - { - m_DropOldConnectionsTimer.Change( HttpConstants.DefaultKeepAliveMilliseconds, System.Threading.Timeout.Infinite ); - } } + } catch { } + } /// @@ -1337,7 +1346,8 @@ private InputNetworkStreamWrapper EstablishConnection(Uri proxyServer, Uri targe try { // If socket is closed ( from this or other side ) the call throws exception. - if (inputStream.m_Socket.Poll(1, SelectMode.SelectWrite)) + //if (inputStream.m_Socket.Poll(1, SelectMode.SelectWrite)) + if (inputStream.m_Socket.Poll(1500000, SelectMode.SelectWrite)) // skigrinder - 2020-12-10 use a larger timeout { // No exception, good we can condtinue and re-use connected stream. @@ -1415,10 +1425,24 @@ 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); - + // skigrinder - 2020-12-10 added exception handling + Socket socket = null; try { + socket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp); + } + catch + { + } + if (socket.SocketType == SocketType.Unknown) + { + socket.Close(); + retStream = null; + return retStream; + } + + + try { socket.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.ReuseAddress, true); } catch{} @@ -1433,9 +1457,16 @@ private InputNetworkStreamWrapper EstablishConnection(Uri proxyServer, Uri targe { IPEndPoint remoteEP = new IPEndPoint(address, proxyServer.Port); socket.Connect((EndPoint)remoteEP); - } - catch (SocketException e) + // skigrinder - check the newly created socket + if (!socket.Poll(1500000, SelectMode.SelectWrite)) { + socket.Close(); + retStream = null; + return retStream; + } + + } catch (SocketException e) { + socket.Close(); // skigrinder - added this to fix a memory leak throw new WebException("connection failed", e, WebExceptionStatus.ConnectFailure, null); } @@ -1476,14 +1507,14 @@ private InputNetworkStreamWrapper EstablishConnection(Uri proxyServer, Uri targe retStream.m_rmAddrAndPort = m_originalUrl.Host + ":" + m_originalUrl.Port; } - lock (m_ConnectedStreams) - { - m_ConnectedStreams.Add(retStream); + if (m_keepAlive) { // skigrinder - check keepAlive before creating persistent connection - persistent connections are not currently supported + lock (m_ConnectedStreams) { + 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); + // 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); + } } } } @@ -1499,50 +1530,66 @@ private void SubmitRequest() // We have connected socket. Create request stream // If proxy is set - connect to proxy server. - if(m_requestStream == null) + // skigrinder - 2020-12-10 added exception handling + try { - if (m_proxy == null) - { // Direct connection to target server. - m_requestStream = EstablishConnection(m_originalUrl, m_originalUrl); - } - else // Connection through proxy. We create network stream connected to proxy + if (m_requestStream == null) { - Uri proxyUri = m_proxy.GetProxy(m_originalUrl); - - if (m_originalUrl.Scheme == "https") - { - // For HTTPs we still need to know the target name to decide on persistent connection. - m_requestStream = EstablishConnection(proxyUri, m_originalUrl); + if (m_proxy == null) + { // Direct connection to target server. + m_requestStream = EstablishConnection(m_originalUrl, m_originalUrl); } - else + else // Connection through proxy. We create network stream connected to proxy { - // For normal HTTP all requests go to proxy - m_requestStream = EstablishConnection(proxyUri, proxyUri); + Uri proxyUri = m_proxy.GetProxy(m_originalUrl); + + if (m_originalUrl.Scheme == "https") + { + // For HTTPs we still need to know the target name to decide on persistent connection. + m_requestStream = EstablishConnection(proxyUri, m_originalUrl); + } + else + { + // For normal HTTP all requests go to proxy + m_requestStream = EstablishConnection(proxyUri, proxyUri); + } } } - } - // We have connected stream. Set the timeout from HttpWebRequest - m_requestStream.WriteTimeout = m_readWriteTimeout; - m_requestStream.ReadTimeout = m_readWriteTimeout; + // skigrinder - 2020-12-10 make sure m_requestStream is valid + 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; - // Now we need to write headers. First we update headers. - PrepareHeaders(); + // Now we need to write headers. First we update headers. + PrepareHeaders(); - // Now send request string and headers. - byte[] dataToSend = GetHTTPRequestData(); + // Now send request string and headers. + byte[] dataToSend = GetHTTPRequestData(); #if DEBUG // In debug mode print the request. It helps a lot to troubleshoot the issues. - int byteUsed, charUsed; - bool completed = false; - char[] charBuf = new char[dataToSend.Length]; - UTF8decoder.Convert(dataToSend, 0, dataToSend.Length, charBuf, 0, charBuf.Length, true, out byteUsed, out charUsed, out completed); - string strSend = new string(charBuf); - Console.WriteLine(strSend); + int byteUsed, charUsed; + bool completed = false; + char[] charBuf = new char[dataToSend.Length]; + UTF8decoder.Convert(dataToSend, 0, dataToSend.Length, charBuf, 0, charBuf.Length, true, out byteUsed, out charUsed, out completed); + string strSend = new string(charBuf); + Console.WriteLine(strSend); #endif - // Writes this data to the network stream. - m_requestStream.Write(dataToSend, 0, dataToSend.Length); - m_requestSent = true; + // Writes this data to the network stream. + m_requestStream.Write(dataToSend, 0, dataToSend.Length); + m_requestSent = true; + } + catch + { + } + } @@ -1725,6 +1772,12 @@ public override WebResponse GetResponse() SubmitRequest(); } + // skigrinder - 2020-12-10 check m_requestSent after SubmitRequest() + if (!m_requestSent) + { + return response; + } + CoreResponseData respData = null; // reset the total response bytes for the new request. @@ -1765,8 +1818,9 @@ public override WebResponse GetResponse() m_responseStatus = response.StatusCode; m_responseCreated = true; + m_requestStream.m_InUse = false; // skigrinder - persistent connections wouldn't work without this - persistent connections are not supported at the moment will hopefully fix this soon } - catch(SocketException se) + catch (SocketException se) { if (m_requestStream != null) { diff --git a/nanoFramework.System.Net.Http/Http/System.Net._OutputNetworkStreamWrapper.cs b/nanoFramework.System.Net.Http/Http/System.Net._OutputNetworkStreamWrapper.cs index d5b827eb..9899275b 100644 --- a/nanoFramework.System.Net.Http/Http/System.Net._OutputNetworkStreamWrapper.cs +++ b/nanoFramework.System.Net.Http/Http/System.Net._OutputNetworkStreamWrapper.cs @@ -139,13 +139,22 @@ public override void Close() /// public override void Flush() { - if (m_headersSend != null) + // skigrinder - 2020-12-10 added exception handling + try + { + if (m_headersSend != null) + { + // Calls HttpListenerResponse.SendHeaders. HttpListenerResponse.SendHeaders sets m_headersSend to null. + m_headersSend(); + } + if (m_Stream != null) + { + m_Stream.Flush(); + } + } + catch { - // Calls HttpListenerResponse.SendHeaders. HttpListenerResponse.SendHeaders sets m_headersSend to null. - m_headersSend(); } - - m_Stream.Flush(); } /// From de5680cd68e744782b010caf7a7cb62d89b9f7f0 Mon Sep 17 00:00:00 2001 From: skigrinder <62192313+skigrinder@users.noreply.github.com> Date: Thu, 17 Dec 2020 09:32:58 -0700 Subject: [PATCH 2/8] Code & comment cleanup as per Jose's comments --- .../Http/System.Net.HttpWebRequest.cs | 55 ++++++++++++------- .../System.Net._OutputNetworkStreamWrapper.cs | 3 +- 2 files changed, 36 insertions(+), 22 deletions(-) diff --git a/nanoFramework.System.Net.Http/Http/System.Net.HttpWebRequest.cs b/nanoFramework.System.Net.Http/Http/System.Net.HttpWebRequest.cs index f2824999..b6b20af4 100644 --- a/nanoFramework.System.Net.Http/Http/System.Net.HttpWebRequest.cs +++ b/nanoFramework.System.Net.Http/Http/System.Net.HttpWebRequest.cs @@ -73,7 +73,8 @@ public class HttpWebRequest : WebRequest /// Unused static private void CheckPersistentConnections(object unused) { - // skigrinder - 2020-12-10 added exception handling & fixed 'timePassed' setting + // Handle exceptions here - Uncaught exceptions were crashing nanoFramework applications + // Persistent connections have not been properly implemented yet. try { int count = m_ConnectedStreams.Count; @@ -89,29 +90,35 @@ static private void CheckPersistentConnections(object unused) try { 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 + TimeSpan timePassed = curTime - streamWrapper.m_lastUsed; // original code was (m_lastUsed - curTime) - good evidence that persistent connections were never implemented // 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 - // - good indication that persistent connections was never implemented in this library + if (timePassed.TotalMilliseconds > HttpConstants.DefaultKeepAliveMilliseconds) // original code used timePassed.Milliseconds - need to use TotalMilliseconds - this will be important if/when persistent connections are implemented { 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) - { - m_DropOldConnectionsTimer.Change(HttpConstants.DefaultKeepAliveMilliseconds, System.Threading.Timeout.Infinite); - } } catch { + // Will need to take action here if/when persistent connections are implemented + // For now, just remove the stream from the persistent connections list. This may not always be appropriate when persistent connections are implemented. m_ConnectedStreams.RemoveAt(i); } } + // turn off the timer if there are no active streams + if (m_ConnectedStreams.Count > 0) + { + m_DropOldConnectionsTimer.Change(HttpConstants.DefaultKeepAliveMilliseconds, System.Threading.Timeout.Infinite); + } + } } - } catch { + } + catch + { + // Uncaught exceptions were crashing nanoFramework applications + // Will need to take action here if/when persistent connections are implemented } } @@ -1347,7 +1354,7 @@ private InputNetworkStreamWrapper EstablishConnection(Uri proxyServer, Uri targe { // If socket is closed ( from this or other side ) the call throws exception. //if (inputStream.m_Socket.Poll(1, SelectMode.SelectWrite)) - if (inputStream.m_Socket.Poll(1500000, SelectMode.SelectWrite)) // skigrinder - 2020-12-10 use a larger timeout + if (inputStream.m_Socket.Poll(1500000, SelectMode.SelectWrite)) // Use a larger timeout { // No exception, good we can condtinue and re-use connected stream. @@ -1425,7 +1432,7 @@ private InputNetworkStreamWrapper EstablishConnection(Uri proxyServer, Uri targe } // If socket was not found in waiting connections, then we create new one. - // skigrinder - 2020-12-10 added exception handling + // Uncaught exceptions in Socket() were causing nanoFramework applications to crash Socket socket = null; try { @@ -1433,6 +1440,8 @@ private InputNetworkStreamWrapper EstablishConnection(Uri proxyServer, Uri targe } catch { + // Uncaught exceptions in Socket() were causing nanoFramework applications to crash + } if (socket.SocketType == SocketType.Unknown) { @@ -1457,7 +1466,7 @@ private InputNetworkStreamWrapper EstablishConnection(Uri proxyServer, Uri targe { IPEndPoint remoteEP = new IPEndPoint(address, proxyServer.Port); socket.Connect((EndPoint)remoteEP); - // skigrinder - check the newly created socket + // Check the newly created socket if (!socket.Poll(1500000, SelectMode.SelectWrite)) { socket.Close(); retStream = null; @@ -1466,7 +1475,7 @@ private InputNetworkStreamWrapper EstablishConnection(Uri proxyServer, Uri targe } catch (SocketException e) { - socket.Close(); // skigrinder - added this to fix a memory leak + socket.Close(); // This fixed a memory leak throw new WebException("connection failed", e, WebExceptionStatus.ConnectFailure, null); } @@ -1507,12 +1516,16 @@ private InputNetworkStreamWrapper EstablishConnection(Uri proxyServer, Uri targe retStream.m_rmAddrAndPort = m_originalUrl.Host + ":" + m_originalUrl.Port; } - if (m_keepAlive) { // skigrinder - check keepAlive before creating persistent connection - persistent connections are not currently supported - lock (m_ConnectedStreams) { + // Check keepAlive before creating persistent connection - persistent connections are not currently supported + if (m_keepAlive) + { + lock (m_ConnectedStreams) + { m_ConnectedStreams.Add(retStream); // if the current stream list is empty then start the timer that drops unused connections. - if (m_ConnectedStreams.Count == 1) { + if (m_ConnectedStreams.Count == 1) + { m_DropOldConnectionsTimer.Change(HttpConstants.DefaultKeepAliveMilliseconds, System.Threading.Timeout.Infinite); } } @@ -1530,7 +1543,7 @@ private void SubmitRequest() // We have connected socket. Create request stream // If proxy is set - connect to proxy server. - // skigrinder - 2020-12-10 added exception handling + // Exception handling here is important - nanoFramework applications were crashing due to unhandled exceptions try { if (m_requestStream == null) @@ -1556,7 +1569,6 @@ private void SubmitRequest() } } - // skigrinder - 2020-12-10 make sure m_requestStream is valid if (m_requestStream == null) { // Connection could not be established @@ -1588,6 +1600,7 @@ private void SubmitRequest() } catch { + // nanoFramework applications were crashing due to unhandled exceptions. } } @@ -1772,7 +1785,7 @@ public override WebResponse GetResponse() SubmitRequest(); } - // skigrinder - 2020-12-10 check m_requestSent after SubmitRequest() + // Need to check m_requestSent after SubmitRequest() if (!m_requestSent) { return response; @@ -1818,7 +1831,7 @@ public override WebResponse GetResponse() m_responseStatus = response.StatusCode; m_responseCreated = true; - m_requestStream.m_InUse = false; // skigrinder - persistent connections wouldn't work without this - persistent connections are not supported at the moment will hopefully fix this soon + m_requestStream.m_InUse = false; // Persistent connections wouldn't work without this - persistent connections are not supported at the moment but will hopefully be fixed soon } catch (SocketException se) { diff --git a/nanoFramework.System.Net.Http/Http/System.Net._OutputNetworkStreamWrapper.cs b/nanoFramework.System.Net.Http/Http/System.Net._OutputNetworkStreamWrapper.cs index 9899275b..5369aecc 100644 --- a/nanoFramework.System.Net.Http/Http/System.Net._OutputNetworkStreamWrapper.cs +++ b/nanoFramework.System.Net.Http/Http/System.Net._OutputNetworkStreamWrapper.cs @@ -139,7 +139,7 @@ public override void Close() /// public override void Flush() { - // skigrinder - 2020-12-10 added exception handling + // Unhandled exceptions here were causing nanoFramework applications to crash try { if (m_headersSend != null) @@ -154,6 +154,7 @@ public override void Flush() } catch { + // Unhandled exceptions here were causing nanoFramework applications to crash } } From ff352013eb1f6e602a13c1848fa04423a1f2e83b Mon Sep 17 00:00:00 2001 From: skigrinder <62192313+skigrinder@users.noreply.github.com> Date: Sun, 20 Dec 2020 08:15:09 -0700 Subject: [PATCH 3/8] Removed try/catch blocks to allow normal .NET exception propagation. --- .../Http/System.Net.HttpWebRequest.cs | 186 +++++++----------- .../System.Net._OutputNetworkStreamWrapper.cs | 18 +- 2 files changed, 72 insertions(+), 132 deletions(-) diff --git a/nanoFramework.System.Net.Http/Http/System.Net.HttpWebRequest.cs b/nanoFramework.System.Net.Http/Http/System.Net.HttpWebRequest.cs index b6b20af4..ccfb30a8 100644 --- a/nanoFramework.System.Net.Http/Http/System.Net.HttpWebRequest.cs +++ b/nanoFramework.System.Net.Http/Http/System.Net.HttpWebRequest.cs @@ -73,54 +73,35 @@ public class HttpWebRequest : WebRequest /// Unused static private void CheckPersistentConnections(object unused) { - // Handle exceptions here - Uncaught exceptions were crashing nanoFramework applications // Persistent connections have not been properly implemented yet. - try + int count = m_ConnectedStreams.Count; + // The fastest way to exit out - if there are no sockets in the list - exit out. + if (count > 0) { - 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) { - DateTime curTime = DateTime.UtcNow; - lock (m_ConnectedStreams) + count = m_ConnectedStreams.Count; + for (int i = count - 1; i >= 0; i--) { - count = m_ConnectedStreams.Count; - for (int i = count - 1; i >= 0; i--) - { - try - { - InputNetworkStreamWrapper streamWrapper = (InputNetworkStreamWrapper)m_ConnectedStreams[i]; - TimeSpan timePassed = curTime - streamWrapper.m_lastUsed; // original code was (m_lastUsed - curTime) - good evidence that persistent connections were never implemented - // If the socket is old, then close and remove from the list. - if (timePassed.TotalMilliseconds > HttpConstants.DefaultKeepAliveMilliseconds) // original code used timePassed.Milliseconds - need to use TotalMilliseconds - this will be important if/when persistent connections are implemented - { - m_ConnectedStreams.RemoveAt(i); - // Closes the socket to release resources. - streamWrapper.Dispose(); - } - } - catch - { - // Will need to take action here if/when persistent connections are implemented - // For now, just remove the stream from the persistent connections list. This may not always be appropriate when persistent connections are implemented. - m_ConnectedStreams.RemoveAt(i); - } - } - // turn off the timer if there are no active streams - if (m_ConnectedStreams.Count > 0) + InputNetworkStreamWrapper streamWrapper = (InputNetworkStreamWrapper)m_ConnectedStreams[i]; + TimeSpan timePassed = curTime - streamWrapper.m_lastUsed; // original code was (m_lastUsed - curTime) - good evidence that persistent connections were never implemented + // If the socket is old, then close and remove from the list. + if (timePassed.TotalMilliseconds > HttpConstants.DefaultKeepAliveMilliseconds) // original code used timePassed.Milliseconds - need to use TotalMilliseconds - this will be important if/when persistent connections are implemented { - m_DropOldConnectionsTimer.Change(HttpConstants.DefaultKeepAliveMilliseconds, System.Threading.Timeout.Infinite); + 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) + { + m_DropOldConnectionsTimer.Change(HttpConstants.DefaultKeepAliveMilliseconds, System.Threading.Timeout.Infinite); + } + } - } - catch - { - // Uncaught exceptions were crashing nanoFramework applications - // Will need to take action here if/when persistent connections are implemented } - } /// @@ -1352,9 +1333,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 (inputStream.m_Socket.Poll(1500000, SelectMode.SelectWrite)) // Use a larger timeout + // If socket is closed ( from this or other side ) the call throws exception. Original code called Poll() with 1. Should be -1. + if (inputStream.m_Socket.Poll(-1, SelectMode.SelectWrite)) { // No exception, good we can condtinue and re-use connected stream. @@ -1369,7 +1349,6 @@ private InputNetworkStreamWrapper EstablishConnection(Uri proxyServer, Uri targe { removeStreamList.Add(inputStream); } - } catch (Exception) { @@ -1432,26 +1411,11 @@ private InputNetworkStreamWrapper EstablishConnection(Uri proxyServer, Uri targe } // If socket was not found in waiting connections, then we create new one. - // Uncaught exceptions in Socket() were causing nanoFramework applications to crash Socket socket = null; - try - { - socket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp); - } - catch - { - // Uncaught exceptions in Socket() were causing nanoFramework applications to crash + socket = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp); - } - if (socket.SocketType == SocketType.Unknown) + try { - socket.Close(); - retStream = null; - return retStream; - } - - - try { socket.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.ReuseAddress, true); } catch{} @@ -1466,14 +1430,8 @@ private InputNetworkStreamWrapper EstablishConnection(Uri proxyServer, Uri targe { IPEndPoint remoteEP = new IPEndPoint(address, proxyServer.Port); socket.Connect((EndPoint)remoteEP); - // Check the newly created socket - if (!socket.Poll(1500000, SelectMode.SelectWrite)) { - socket.Close(); - retStream = null; - return retStream; - } - - } catch (SocketException e) + } + catch (SocketException e) { socket.Close(); // This fixed a memory leak throw new WebException("connection failed", e, WebExceptionStatus.ConnectFailure, null); @@ -1543,66 +1501,57 @@ private void SubmitRequest() // We have connected socket. Create request stream // If proxy is set - connect to proxy server. - // Exception handling here is important - nanoFramework applications were crashing due to unhandled exceptions - try + if (m_requestStream == null) { - if (m_requestStream == null) + if (m_proxy == null) + { // Direct connection to target server. + m_requestStream = EstablishConnection(m_originalUrl, m_originalUrl); + } + else // Connection through proxy. We create network stream connected to proxy { - if (m_proxy == null) - { // Direct connection to target server. - m_requestStream = EstablishConnection(m_originalUrl, m_originalUrl); + Uri proxyUri = m_proxy.GetProxy(m_originalUrl); + + if (m_originalUrl.Scheme == "https") + { + // For HTTPs we still need to know the target name to decide on persistent connection. + m_requestStream = EstablishConnection(proxyUri, m_originalUrl); } - else // Connection through proxy. We create network stream connected to proxy + else { - Uri proxyUri = m_proxy.GetProxy(m_originalUrl); - - if (m_originalUrl.Scheme == "https") - { - // For HTTPs we still need to know the target name to decide on persistent connection. - m_requestStream = EstablishConnection(proxyUri, m_originalUrl); - } - else - { - // For normal HTTP all requests go to proxy - m_requestStream = EstablishConnection(proxyUri, proxyUri); - } + // For normal HTTP all requests go to proxy + m_requestStream = EstablishConnection(proxyUri, proxyUri); } } + } - if (m_requestStream == null) - { - // Connection could not be established - m_requestSent = false; - return; - } + 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; + // We have connected stream. Set the timeout from HttpWebRequest + m_requestStream.WriteTimeout = m_readWriteTimeout; + m_requestStream.ReadTimeout = m_readWriteTimeout; - // Now we need to write headers. First we update headers. - PrepareHeaders(); + // Now we need to write headers. First we update headers. + PrepareHeaders(); - // Now send request string and headers. - byte[] dataToSend = GetHTTPRequestData(); + // Now send request string and headers. + byte[] dataToSend = GetHTTPRequestData(); #if DEBUG // In debug mode print the request. It helps a lot to troubleshoot the issues. - int byteUsed, charUsed; - bool completed = false; - char[] charBuf = new char[dataToSend.Length]; - UTF8decoder.Convert(dataToSend, 0, dataToSend.Length, charBuf, 0, charBuf.Length, true, out byteUsed, out charUsed, out completed); - string strSend = new string(charBuf); - Console.WriteLine(strSend); + int byteUsed, charUsed; + bool completed = false; + char[] charBuf = new char[dataToSend.Length]; + UTF8decoder.Convert(dataToSend, 0, dataToSend.Length, charBuf, 0, charBuf.Length, true, out byteUsed, out charUsed, out completed); + string strSend = new string(charBuf); + Console.WriteLine(strSend); #endif - // Writes this data to the network stream. - m_requestStream.Write(dataToSend, 0, dataToSend.Length); - m_requestSent = true; - } - catch - { - // nanoFramework applications were crashing due to unhandled exceptions. - } - + // Writes this data to the network stream. + m_requestStream.Write(dataToSend, 0, dataToSend.Length); + m_requestSent = true; } @@ -1844,12 +1793,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); } diff --git a/nanoFramework.System.Net.Http/Http/System.Net._OutputNetworkStreamWrapper.cs b/nanoFramework.System.Net.Http/Http/System.Net._OutputNetworkStreamWrapper.cs index 5369aecc..960e40e5 100644 --- a/nanoFramework.System.Net.Http/Http/System.Net._OutputNetworkStreamWrapper.cs +++ b/nanoFramework.System.Net.Http/Http/System.Net._OutputNetworkStreamWrapper.cs @@ -139,22 +139,14 @@ public override void Close() /// public override void Flush() { - // Unhandled exceptions here were causing nanoFramework applications to crash - try + if (m_headersSend != null) { - if (m_headersSend != null) - { - // Calls HttpListenerResponse.SendHeaders. HttpListenerResponse.SendHeaders sets m_headersSend to null. - m_headersSend(); - } - if (m_Stream != null) - { - m_Stream.Flush(); - } + // Calls HttpListenerResponse.SendHeaders. HttpListenerResponse.SendHeaders sets m_headersSend to null. + m_headersSend(); } - catch + if (m_Stream != null) // Need to check for null before using here { - // Unhandled exceptions here were causing nanoFramework applications to crash + m_Stream.Flush(); } } From dea00a7314a48eadb31e1d73e1c85eae3356f718 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Sim=C3=B5es?= Date: Tue, 22 Dec 2020 01:15:24 +0000 Subject: [PATCH 4/8] Mostly code style fixes --- .../Http/System.Net.HttpWebRequest.cs | 24 ++++++++++++++----- 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/nanoFramework.System.Net.Http/Http/System.Net.HttpWebRequest.cs b/nanoFramework.System.Net.Http/Http/System.Net.HttpWebRequest.cs index ccfb30a8..7a0f3557 100644 --- a/nanoFramework.System.Net.Http/Http/System.Net.HttpWebRequest.cs +++ b/nanoFramework.System.Net.Http/Http/System.Net.HttpWebRequest.cs @@ -75,25 +75,32 @@ 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--) { InputNetworkStreamWrapper streamWrapper = (InputNetworkStreamWrapper)m_ConnectedStreams[i]; - TimeSpan timePassed = curTime - streamWrapper.m_lastUsed; // original code was (m_lastUsed - curTime) - good evidence that persistent connections were never implemented + + TimeSpan timePassed = curTime - streamWrapper.m_lastUsed; + // If the socket is old, then close and remove from the list. - if (timePassed.TotalMilliseconds > HttpConstants.DefaultKeepAliveMilliseconds) // original code used timePassed.Milliseconds - need to use TotalMilliseconds - this will be important if/when persistent connections are implemented + 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) { @@ -1333,7 +1340,7 @@ 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. Original code called Poll() with 1. Should be -1. + // 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. @@ -1419,6 +1426,7 @@ private InputNetworkStreamWrapper EstablishConnection(Uri proxyServer, Uri targe socket.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.ReuseAddress, true); } catch{} + try { socket.SetSocketOption(SocketOptionLevel.Tcp, SocketOptionName.NoDelay, true); @@ -1433,7 +1441,9 @@ private InputNetworkStreamWrapper EstablishConnection(Uri proxyServer, Uri targe } catch (SocketException e) { - socket.Close(); // This fixed a memory leak + // need to close socket, otherwise this will cause an out of memory exception + socket.Close(); + throw new WebException("connection failed", e, WebExceptionStatus.ConnectFailure, null); } @@ -1504,11 +1514,13 @@ private void SubmitRequest() 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") From 5ff91f8c9022bdab23ac003a5b17d4f53fa62a24 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Sim=C3=B5es?= Date: Sun, 27 Dec 2020 12:36:29 +0000 Subject: [PATCH 5/8] Update System.Net._OutputNetworkStreamWrapper.cs --- .../Http/System.Net._OutputNetworkStreamWrapper.cs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/nanoFramework.System.Net.Http/Http/System.Net._OutputNetworkStreamWrapper.cs b/nanoFramework.System.Net.Http/Http/System.Net._OutputNetworkStreamWrapper.cs index 960e40e5..46f0b1da 100644 --- a/nanoFramework.System.Net.Http/Http/System.Net._OutputNetworkStreamWrapper.cs +++ b/nanoFramework.System.Net.Http/Http/System.Net._OutputNetworkStreamWrapper.cs @@ -144,10 +144,9 @@ public override void Flush() // Calls HttpListenerResponse.SendHeaders. HttpListenerResponse.SendHeaders sets m_headersSend to null. m_headersSend(); } - if (m_Stream != null) // Need to check for null before using here - { - m_Stream.Flush(); - } + + // Need to check for null before using here + m_Stream?.Flush(); } /// From 01ff6ebbc346950dcdd1ee3ad2050fba3993f9b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Sim=C3=B5es?= Date: Mon, 28 Dec 2020 11:55:36 +0000 Subject: [PATCH 6/8] Update System.Net.HttpWebRequest.cs --- .../Http/System.Net.HttpWebRequest.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/nanoFramework.System.Net.Http/Http/System.Net.HttpWebRequest.cs b/nanoFramework.System.Net.Http/Http/System.Net.HttpWebRequest.cs index 7a0f3557..947eaf80 100644 --- a/nanoFramework.System.Net.Http/Http/System.Net.HttpWebRequest.cs +++ b/nanoFramework.System.Net.Http/Http/System.Net.HttpWebRequest.cs @@ -88,9 +88,9 @@ static private void CheckPersistentConnections(object unused) for (int i = count - 1; i >= 0; i--) { InputNetworkStreamWrapper streamWrapper = (InputNetworkStreamWrapper)m_ConnectedStreams[i]; - + TimeSpan timePassed = curTime - streamWrapper.m_lastUsed; - + // If the socket is old, then close and remove from the list. if (timePassed.TotalMilliseconds > HttpConstants.DefaultKeepAliveMilliseconds) { @@ -100,7 +100,7 @@ static private void CheckPersistentConnections(object unused) streamWrapper.Dispose(); } } - + // turn off the timer if there are no active streams if (m_ConnectedStreams.Count > 0) { From b6adf9eb9923c3bf8d80c6e4b897b7bd04655201 Mon Sep 17 00:00:00 2001 From: Robin Jones Date: Mon, 28 Dec 2020 12:34:41 +0000 Subject: [PATCH 7/8] Remove un-necessary whitespace --- nanoFramework.System.Net.Http/Http/System.Net.HttpWebRequest.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nanoFramework.System.Net.Http/Http/System.Net.HttpWebRequest.cs b/nanoFramework.System.Net.Http/Http/System.Net.HttpWebRequest.cs index 947eaf80..589e10c8 100644 --- a/nanoFramework.System.Net.Http/Http/System.Net.HttpWebRequest.cs +++ b/nanoFramework.System.Net.Http/Http/System.Net.HttpWebRequest.cs @@ -102,7 +102,7 @@ static private void CheckPersistentConnections(object unused) } // 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); } From f83bea58e9930d4f539e93e09351465df584fc44 Mon Sep 17 00:00:00 2001 From: Robin Jones Date: Mon, 28 Dec 2020 12:51:51 +0000 Subject: [PATCH 8/8] Improve comment. --- nanoFramework.System.Net.Http/Http/System.Net.HttpWebRequest.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nanoFramework.System.Net.Http/Http/System.Net.HttpWebRequest.cs b/nanoFramework.System.Net.Http/Http/System.Net.HttpWebRequest.cs index 589e10c8..e1597a03 100644 --- a/nanoFramework.System.Net.Http/Http/System.Net.HttpWebRequest.cs +++ b/nanoFramework.System.Net.Http/Http/System.Net.HttpWebRequest.cs @@ -1792,7 +1792,7 @@ public override WebResponse GetResponse() m_responseStatus = response.StatusCode; m_responseCreated = true; - m_requestStream.m_InUse = false; // Persistent connections wouldn't work without this - persistent connections are not supported at the moment but will hopefully be fixed soon + m_requestStream.m_InUse = false; // Persistent connections are not yet supported, but they wouldn't work without this. } catch (SocketException se) {