search

rebus-org / Rebus.Oracle

:bus: Oracle transport for Rebus
https://mookid.dk/category/rebus
Other
5 stars 10 forks source link

Don't use async Oracle methods #17

Closed jods4 closed 5 years ago

jods4 commented 5 years ago

⚠️ Tests not run yet (no access to an Oracle DB atm)

They are not implemented and fallback to wrapping sync ones.

Using async calls with Oracle.ManagedDataAccess only results in more allocations (tasks, async state machines, etc) and more complex code; yet yields no benefit since they all block and complete synchronously.

This change enabled me to get rid of a reference to SQL Server assembly, which is not desirable in this project.

Fixes #12

mookid8000 commented 5 years ago

Thanks for all the PRs 😁 👍

I'll release it as Rebus.Oracle/Rebus.Oracle.Devart 2.0.0-a3 on Nuget.org now

jods4 commented 5 years ago

@mookid8000 did you run the tests? I wanted to run them today but didn't have the time to. If you didn't I'll run them tomorrow.

mookid8000 commented 5 years ago

I didn’t 😳🙄

Sendt fra min iPhone

– Mogens Heller Grabe Torsmark 4 8700 Horsens Danmark

+45 29367077 mookid8000@gmail.com

Den 12. aug. 2019 kl. 20.35 skrev jods notifications@github.com:

@mookid8000 did you run the tests? I wanted to run them today but didn't have the time to. If you didn't I'll run them tomorrow.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

jods4 commented 5 years ago

OK. I'll run them tomorrow.

Also notice I only touched Rebus.Oracle, I'm not changing anything to Rebus.Oracle.Devart (so no need to publish a new version on Nuget).

Also also: I'm planning on submitting more (databus notably), so if you want to wait before publishing on Nuget that's ok.

jods4 commented 5 years ago

FYI I ran tests on Oracle 11:

Total tests: 70
     Passed: 67
     Failed: 2
    Skipped: 1
 Total time: 2.5039 Minutes

😞 I'm really not sure that the 2 failures were caused by this PR but I'll check. Not sure either why 1 test is skipped, I'll check as well.

mookid8000 commented 5 years ago

I'm really not sure that the 2 failures were caused by this PR but I'll check. Not sure either why 1 test is skipped, I'll check as well.

cool, thanks!

jods4 commented 5 years ago

Silly me. The run above was against master on my fork, so actually it was before any of my changes. Well, that gives me a baseline at least.

I did a new run against the merged changes and it's worse. 

Total tests: 70
     Passed: 61
     Failed: 8
    Skipped: 1
 Total time: 2.4281 Minutes
jods4 commented 5 years ago

OK so regressions are caused by different exception types.

Because the methods are now sync (they just return Task.CompletedTask) they throw specific exceptions whereas tests expect AggregateException, which is what is thrown when you put await into the mix.

The AggregateException part is always a little annoying when working with async. If you don't fork then I think it's easier to work with the unwrapped exceptions, but anyway to keep things simple I will wrap all exceptions into AggregateException.

jods4 commented 5 years ago

More investigation on tests that were broken before: