Moving Fiber creation up before middleware chain invoked #58
Conversation
enable async middleware to make Fiber-aware aynchronous calls during pre-processing
|
Hmm, this looks sane. @dj2: you looked into this in the past, any reason why we didn't do this earlier? |
|
It seemed the right approach, the key feedback I was interested in is whether the Fiber creation is in the right place. I spent quite a bit of time following the request flow and it seemed the best place. |
|
Yep, no the actual change makes perfect sense to me. The only reason I ask is because we talked about exactly this changeset with @dj2 a while back at the office, and he was going to play with it and commit it, if it made sense.. I'm just not sure if he ever got around to it, and if yes, then why we he didn't push. I'll wait for Dan to confirm/deny... :) |
|
I made the change, it seemed to work. We talked about the potential performance of having to create the fiber before running the middlewares, which I believe we decided was fine. I then promptly forgot about the whole thing..... |
| @@ -58,8 +58,22 @@ module Goliath | |||
| params.merge!(post_params) | |||
| end | |||
|
|
|||
| params | |||
| indifferent_params(params) | |||
There was a problem hiding this comment.
The indifferent_params feels like it should be a different commit from moving the fiber.
There was a problem hiding this comment.
Hmm, that's annoying about the indifferent_params. I didn't realise pushing to my master branch would automatically add them to the pull request
There was a problem hiding this comment.
Yea, anything in that branch will show up. I've found that if you're going to send a pull request it's best to branch it from master first then send a pull for the branch. You can then continue doing stuff in another branch (possibly branched from the pull request branch) but don't do anything in the pull request branch.
There was a problem hiding this comment.
OK, do you want me to create a feature branch for the Fiber change and issue a new request?
Moving Fiber creation up before middleware chain invoked
This has come out of the discussion around being able to use async calls within middleware. I've moved the Fiber creation up from Goliath::API to Goliath::Request. It's still passing all the specs, and I've been running my version for a couple of weeks on a new internal project, but it would be nice to see it tested against some bigger Goliath apps.
Steve