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

A bunch of fixes from production usage #3 #31

Merged
merged 15 commits into from
Nov 21, 2023

Conversation

dimhotepus
Copy link
Contributor

No description provided.

@juliusfriedman
Copy link
Owner

Mostly LGTM, will take another look shortly as I have started rebasing the other branch like 3 times already and I think it will be easier to merge this first.

Thanks for your contributions!

@@ -1327,7 +1320,7 @@ internal void RestartFaultedStreams()
/// Starts the RtspServer and listens for requests.
/// Starts all streams contained in the server
/// </summary>
public virtual void Start(bool allowAddressReuse = false, bool allowPortReuse = false)
public virtual async Task StartAsync(bool allowAddressReuse = false, bool allowPortReuse = false)
Copy link
Owner

Choose a reason for hiding this comment

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

We should probably leave the non async versions or provide a wrapper.

Copy link
Owner

@juliusfriedman juliusfriedman left a comment

Choose a reason for hiding this comment

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

LGTM, we should probably keep the non Async methods so we don't break anyone but I still am going to approve this PR at this time.

Thanks for your contributions.

I will work on reasing the other one shortly.

@juliusfriedman juliusfriedman merged commit 1644ab4 into juliusfriedman:master Nov 21, 2023
3 checks passed
@juliusfriedman
Copy link
Owner

The latest code in this branch has a bug where you can no longer see the names of any test ran. I am looking to into it, it seems the changes are around

RunTestAsync(() => { test(); return Task.CompletedTask; }, count, waitForGoAhead).GetAwaiter().GetResult();

@juliusfriedman
Copy link
Owner

juliusfriedman commented Nov 21, 2023

I fixed it best I could for now, the Method.Name is hard to synthesize from a lambda, I don't want to spend a ton of time on such things. I will start to rebase your other commit now.

If possible, in the future, please make sure all logic runs the same as before your commit (or better after a bug is fixed).

Thanks again for your contributions!!!

@dimhotepus dimhotepus deleted the production-fixes-3 branch November 21, 2023 15:37
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.

2 participants