fiber icon indicating copy to clipboard operation
fiber copied to clipboard

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

Open akshaybharambe14 opened this issue 3 years ago • 19 comments

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

fixes #1496

akshaybharambe14 avatar Oct 02 '21 06:10 akshaybharambe14

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

welcome[bot] avatar Oct 02 '21 06:10 welcome[bot]

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

ReneWerner87 avatar Oct 04 '21 06:10 ReneWerner87

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

akshaybharambe14 avatar Oct 04 '21 06:10 akshaybharambe14

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

ReneWerner87 avatar Oct 04 '21 06:10 ReneWerner87

Yup, I will work on that this weekend.

akshaybharambe14 avatar Oct 05 '21 03:10 akshaybharambe14

@akshaybharambe14 any progress ?

ReneWerner87 avatar Oct 11 '21 06:10 ReneWerner87

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.

akshaybharambe14 avatar Oct 11 '21 13:10 akshaybharambe14

@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

ReneWerner87 avatar Oct 11 '21 16:10 ReneWerner87

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

akshaybharambe14 avatar Oct 13 '21 04:10 akshaybharambe14

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

akshaybharambe14 avatar Oct 23 '21 06:10 akshaybharambe14

Thx

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

ReneWerner87 avatar Oct 23 '21 07:10 ReneWerner87

please check the data race image

ReneWerner87 avatar Oct 24 '21 07:10 ReneWerner87

@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

ReneWerner87 avatar Oct 28 '21 06:10 ReneWerner87

I will check that @ReneWerner87.

akshaybharambe14 avatar Oct 29 '21 14:10 akshaybharambe14

@ReneWerner87, is there any way to copy ctx?

Seems like, we need to copy Ctx fields.

akshaybharambe14 avatar Oct 30 '21 06:10 akshaybharambe14

@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 avatar Dec 28 '21 13:12 ReneWerner87

@akshaybharambe14

ReneWerner87 avatar Jan 21 '22 11:01 ReneWerner87

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

ReneWerner87 avatar Apr 05 '22 06:04 ReneWerner87

When will this issue be resolved and released?

kevin-you2 avatar Jun 30 '22 09:06 kevin-you2

Fixed by https://github.com/gofiber/fiber/pull/2090

efectn avatar Sep 16 '22 14:09 efectn