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.

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

[browser][websockets][tests] Intermittent Test failures Uncaught RuntimeError: memory access out of bounds #45586

Closed
kjpou1 opened this issue Dec 4, 2020 · 1 comment · Fixed by #47906
Assignees
Labels
arch-wasm WebAssembly architecture area-System.Net
Milestone

Comments

@kjpou1
Copy link
Contributor

kjpou1 commented Dec 4, 2020

Some tests are throwing an intermittent RuntimeError: memory access out of bounds.

Hard to track down where this is being thrown: In the test framework or websocket code is causing this.

Update:

Most of these stem from not closing the connection correctly which keeps the session open.

These are being identified and worked through. AbortTests are the common cause #45674

@kjpou1 kjpou1 added arch-wasm WebAssembly architecture area-System.Net labels Dec 4, 2020
@kjpou1 kjpou1 self-assigned this Dec 4, 2020
@ghost
Copy link

ghost commented Dec 4, 2020

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Some tests are throwing an intermittent RuntimeError: memory access out of bounds.

Hard to track down where this is being thrown: In the test framework or websocket code is causing this.

Author: kjpou1
Assignees: kjpou1
Labels:

arch-wasm, area-System.Net

Milestone: -

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Dec 4, 2020
@kjpou1 kjpou1 removed the untriaged New issue has not been triaged by the area owner label Dec 4, 2020
@kjpou1 kjpou1 added this to the 6.0.0 milestone Jan 5, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 18, 2021
kjpou1 added a commit that referenced this issue Feb 11, 2021
* Set the WebSocketException to Faulted to fix tests

* Fix the exceptions that are being thrown to coincide with test expectations

* Fix Dispose we are not processing it multiple times.

* Fix Abort code so the messages align correctly with the tests.

example from tests:

```
Assert.Equal() Failure
  info: Expected: Aborted
  info: Actual:   Closed
```

* Set the WebSocketException to Faulted to fix test expectations

* Close the connections correctly.

* Fix Abort test Abort_CloseAndAbort_Success

- This was causing a never ending Task when aborted after a Close already executed.
- Never ended which was a cause of memory errors after left running.
- See also #45586

* - Fixes for ReceiveAsyncTest

- Exception text not sent correctly.  This test was expecting 'Aborted'.
- Mismatched expected exception messages
- Make sure the connection is torn down.
- Multiple GC calls that end in running out of memory fixed.  #45586

```
//  [FAIL] System.Net.WebSockets.Client.Tests.CancelTest.ReceiveAsync_AfterCancellationDoReceiveAsync_ThrowsWebSocketException(server: ws://corefx-net-http11.azurewebsites.net/WebSocket/EchoWebSocket.ashx)
//   info: Assert.Equal() Failure
//   info:                                  ↓ (pos 39)
//   info: Expected: ··· an invalid state ('Aborted') for this operation. Valid state···
//   info: Actual:   ··· an invalid state ('Open') for this operation. Valid states a···
//   info:                                  ↑ (pos 39)

```

* Remove ActiveIssue attributes that should be fixed

* Add back code after merge conflict

- Update tests that are resolved.

* Fix tests that were failing when expecting CloseSent as a valid state.

```
// fail: [FAIL] System.Net.WebSockets.Client.Tests(server: ws://corefx-net-http11.azurewebsites.net/WebSocket/EchoWebSocket.ashx)
//   info: Assert.Throws() Failure
//   info: Expected: typeof(System.OperationCanceledException)
//   info: Actual:   typeof(System.Net.WebSockets.WebSocketException): The WebSocket is in an invalid state ('Aborted') for this operation. Valid states are: 'Open, CloseSent'
```

* Fix the Abort and Cancel never ending tests.

- Fix the clean up of the task and set or cancel the task cleanly.

* Add ActiveIssue to websocket client SendRecieve tests

* Add ActiveIssue to websocket client SendRecieve tests

- intermittently failing with System.OperationCanceledException : The operation was canceled.

* Add extra time to timeout for a couple of tests that were intermittently failing with System.OperationCanceledException : The operation was canceled.

- This was an ActiveIssue #46909 but extending the time seems to do the job.

* Add ActiveIssue to websocket client SendRecieve tests

- [browser][websocket] Hang with responses without ever signaling "end of message"
- [ActiveIssue("#46983", TestPlatforms.Browser)]  // JS Fetch does not support see issue

* Remove `Interlocked` code as per review comment.

* Fix comment

* Fix Abort tests on non chrome browsers.

- Fix for those browsers that do not set Close and send an onClose event in certain instances i.e. firefox and safari.
- Chrome will send an onClose event and we tear down the websocket there.  Other browsers need to be handled differently.

* We should not throw here.

- Closing an already-closed ClientWebSocket should be a no-op.

* Add `CloseOutputAsync` implementation

- The browser websocket implementation does not support this so we fake it the best we can.

* Remove `ActiveIssue` and add more tests

- Remove `[ActiveIssue("#45468", TestPlatforms.Browser)]`
- Add more tests

* Address timeout implementation review

* Add code as per SignalR comments to prevent long wait times in some cases

As per comments
- We clear all events on the websocket (including onClose),
- call websocket.close()
- then call the user provided onClose method manually.

* Update src/libraries/System.Net.WebSockets.Client/tests/SendReceiveTest.cs

Co-authored-by: Larry Ewing <lewing@microsoft.com>

* Update src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs

Co-authored-by: Stephen Toub <stoub@microsoft.com>

* Update src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs

Co-authored-by: Stephen Toub <stoub@microsoft.com>

* Address review comments for using `TrySet*` variants

* Address review about using PascalCasing

* Change test to be a platform check and not an ActiveIssue as per review

Co-authored-by: Larry Ewing <lewing@microsoft.com>
Co-authored-by: Stephen Toub <stoub@microsoft.com>
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Feb 11, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Mar 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-System.Net
Projects
None yet
2 participants