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

[dotnet] Send data over cdp consecutively #12591

Merged
merged 10 commits into from
Aug 31, 2023
Merged

[dotnet] Send data over cdp consecutively #12591

merged 10 commits into from
Aug 31, 2023

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Aug 22, 2023

Description

According https://learn.microsoft.com/en-us/dotnet/api/system.net.websockets.websocket

Issuing multiple sends or multiple receives at the same time (for example, without awaiting, or from multiple threads without synchronization) is not supported and will result in an undefined behavior. Ensure that the previous operation is awaited (or completed) before issuing the next one. Serialize the access via whatever mechanism works best for you, for example, by using a lock or a semaphore.

Here we ensure that we allow to send data consecutively using SemaphoreSlim as synchronization context.

Motivation and Context

Fixes #10054

Types of changes

  • 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)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@nvborisenko
Copy link
Member Author

I didn't test it locally, my build is failing. Hopefully I can grab artifacts from CI and test it.

@nvborisenko
Copy link
Member Author

Perfect, I was able to build locally with --stamp flag. This PR fixes the issue, verified locally.

@titusfortner
Copy link
Member

@nvborisenko can you build selenium manager locally? That's the big difference between stamp and not stamp. @p0deje this is probably why we had it set that way in CI?

@nvborisenko
Copy link
Member Author

I remember --stamp flag takes pre-build selenium manager, which allowed me to build modified C# code to verify my changes. Please ping me in chat, I will share all details you need about how build process works on my environment.

@p0deje
Copy link
Member

p0deje commented Aug 22, 2023

@titusfortner Looking at https://github.com/SeleniumHQ/selenium/actions/runs/5942888108/job/16117259011?pr=12591 - this doesn't seem to be related to Selenium-Manager. @nvborisenko Did you get the same error locally?

@diemol
Copy link
Member

diemol commented Aug 25, 2023

Probably we should hold this and see if it is needed after merging #12614? What do you think, @nvborisenko?

@diemol diemol added this to the 4.12 milestone Aug 25, 2023
Co-authored-by: Yevgeniy Shunevych <yevgeniy.shunevych@gmail.com>
@nvborisenko
Copy link
Member Author

@diemol @jimevans
#12614 is merged, but I don't see protection of SendData method from concurrent use. There is still possibility to execute this method from different threads which leads to unpredictable behavior.

I think we should adopt this PR and move the protection to new WebSocketConnection class (or layer upper to DevToolsSession class?).

@nvborisenko
Copy link
Member Author

Ok, I adopted it with the latest trunk. This PR is ready for review.

@titusfortner
Copy link
Member

These failures have been determined to be an issue with how we are building the alert page for the test server, so I'm going to merge without these passing.

@titusfortner titusfortner merged commit a6f37ca into SeleniumHQ:trunk Aug 31, 2023
9 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[🐛 Bug]: Selenium 4 WebSocket exception during performing basic authorization C#
6 participants