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

🐛 fix: actual error not forwarded if request is not timed out. #1559

Closed
wants to merge 4 commits into from

Conversation

akshaybharambe14
Copy link

Forwarded the actual error if request succeeds/fails in given timeout window.

fixes #1496

@welcome
Copy link

welcome bot commented Oct 2, 2021

Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

@ReneWerner87
Copy link
Member

@akshaybharambe14 can you provide a test for this customization so we can make sure the workflow works as intended?

@akshaybharambe14
Copy link
Author

I checked existing tests and all were commented out. Anyways, I will add tests.

@ReneWerner87
Copy link
Member

ok thx
you can also try to reactivate the tests, I think someone had deactivated them because of the stability, we would have to try to get them back, best as stable as possible

@akshaybharambe14
Copy link
Author

Yup, I will work on that this weekend.

@ReneWerner87
Copy link
Member

@akshaybharambe14 any progress ?

@akshaybharambe14
Copy link
Author

Yes, I added some test cases but but the result is not consistent. Same test that passed previously, might fail in next run. This is because of the select block.
Example -
If we have a handler that takes 3ms and timeout is set for 5ms, we will still get timeout error in some cases.

This could be the reason for commenting out the test cases.

@ReneWerner87
Copy link
Member

@akshaybharambe14 We had the same problems with the caching middleware

Cause is that the github processes or the language is always different speed

I have stabilized this by choosing the times more generously, so that the process has enough room to fluctuate

Can you try this here also, gladly also mark as parallel, if that was not made

@akshaybharambe14
Copy link
Author

Thank you @ReneWerner87 for these insights. I will try to implement the suggestions.

@akshaybharambe14
Copy link
Author

Hi @ReneWerner87, I have added tests. Please have a look.

@ReneWerner87
Copy link
Member

Thx

Had you looked to see if you could correct the other commented out ones as well?

@ReneWerner87
Copy link
Member

please check the data race
image

@ReneWerner87
Copy link
Member

@akshaybharambe14 please check the race condition

think this is because of the access to the parameters, if the request is reused, you can't access the parameters anymore

@akshaybharambe14
Copy link
Author

I will check that @ReneWerner87.

@akshaybharambe14
Copy link
Author

@ReneWerner87, is there any way to copy ctx?

Seems like, we need to copy Ctx fields.

@ReneWerner87
Copy link
Member

ReneWerner87 commented Dec 28, 2021

@ReneWerner87, is there any way to copy ctx?

Seems like, we need to copy Ctx fields.

@akshaybharambe14
not really, but you are welcome to provide a function that can do that, I think something like that would be helpful

@ReneWerner87
Copy link
Member

@akshaybharambe14

@ReneWerner87
Copy link
Member

@akshaybharambe14 do you have time to look at the problem again?

@kevin-you2
Copy link

When will this issue be resolved and released?

@efectn
Copy link
Member

efectn commented Sep 16, 2022

Fixed by #2090

@efectn efectn closed this Sep 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 Timeout middleware is ignoring errors returned by the handler
4 participants