Skip to content

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

Closed
akshaybharambe14 wants to merge 4 commits into
gofiber:masterfrom
akshaybharambe14:fix/1496
Closed

🐛 fix: actual error not forwarded if request is not timed out.#1559
akshaybharambe14 wants to merge 4 commits into
gofiber:masterfrom
akshaybharambe14:fix/1496

Conversation

@akshaybharambe14

Copy link
Copy Markdown

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

fixes #1496

@welcome

welcome Bot commented Oct 2, 2021

Copy link
Copy Markdown

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
Copy Markdown
Member

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

@akshaybharambe14

Copy link
Copy Markdown
Author

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

@ReneWerner87

Copy link
Copy Markdown
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
Copy Markdown
Author

Yup, I will work on that this weekend.

@ReneWerner87

Copy link
Copy Markdown
Member

@akshaybharambe14 any progress ?

@akshaybharambe14

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Author

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

@akshaybharambe14

Copy link
Copy Markdown
Author

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

@ReneWerner87

Copy link
Copy Markdown
Member

Thx

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

@ReneWerner87

Copy link
Copy Markdown
Member

please check the data race
image

@ReneWerner87

Copy link
Copy Markdown
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
Copy Markdown
Author

I will check that @ReneWerner87.

@akshaybharambe14

Copy link
Copy Markdown
Author

@ReneWerner87, is there any way to copy ctx?

Seems like, we need to copy Ctx fields.

@ReneWerner87

ReneWerner87 commented Dec 28, 2021

Copy link
Copy Markdown
Member

@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
Copy Markdown
Member

@akshaybharambe14

@ReneWerner87

Copy link
Copy Markdown
Member

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

@kevin-you2

Copy link
Copy Markdown

When will this issue be resolved and released?

@efectn

efectn commented Sep 16, 2022

Copy link
Copy Markdown
Member

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