`managerModifyRequest` is executed twice for every request

[nikomi]

In responseOpen I think managerModifyRequest is executed twice for every request.
This is the beginning of responseOpen:

  (manager, req0) <- getModifiedRequestManager manager' inputReq
  ...

A bit later the redirection loop is entered where the request is modified again:

      count
      (\req -> do
        (manager, modReq) <- getModifiedRequestManager manager0 req
        ...

This leads to strange effects in our library because modifications are chained - each modification adds a response checker so responses are checked twice, with unpleasant side effects.

If possible (and not required) could you please refactor the code so that each request is only modified once, e.g. by moving the modification code in go after the actual call, so it modifies a redirect request before looping back to execute it?

[snoyberg]

That code is particularly hairy. I'm not sure if there's a way to modify it without negative consequences for some use case.

[nikomi]

I think the amount of hairyness depends on the requirements of mWrapException:
If mWrapException does not need the modified Manager & Request the solution is simple:
Rename the parameters and remove the first line of code, i.e.

responseOpen req0 manager = do
  -- (manager, req0) <- getModifiedRequestManager manager' inputReq
  wrapExc req0 $ mWrapException manager req0 $ do

If mWrapException does need the modified Manager & Request the required change needs to be buried inside the function parameter to httpRedirect' - which I definitely have not enough insight into.

So I can only hope mWrapException does not need the modified M & R - all I can say is the current double application completely breaks our circuit breaker layered on top of http-client.

[snoyberg]

You can look through the Git history of that function; it's set up the way it is now quite intentionally to provide the modified Manager and Request where they're at.

[nikomi]

Ah, I see.

Fortunately there is another possible solution to our dilemma: enable the modifying function to detect if the request has already been modified. Unless I missed something there currently is no clean way to do this, so I would propose the following:

• adding a field to Request that allows potential modifying functions to mark and recognise a Request as having been modified
• since there could be layers of modifying functions this should probably a list (a simple Bool would work for a single modifying function but would not be generic)
• in the vast majority of cases this list would remain empty and is therefore close to zero overhead
• a list of UUIDs would probably be on the safe side (every potential modifying function selects a UUID, the risk of two modifying function selecting the same UUID is really low)
• to make the comparisons a bit cheaper a list of Int might be sufficient - libs using this feature could select and document a large random Int - has slighty higher potential of conflict, but I would assume still rather low

What do you think?

[snoyberg]

I think this is a lot of complexity I wouldn't like to include in the library.

[nikomi]

Then I withdraw the request and remain with version 0.4.

Nonetheless, thanx for looking into this.

[mbj]

I've been bitten by my false assumptions on the behavior of the managerModifyRequest multiple times now. And I always end up reading this thread.

Is it okay I send a PR that documents this behavior? Basically I want to say: Its the job of the author of the modification function to ensure idempotency. I think there is no valid use case where the modification function should not be idempotent?

[snoyberg]

PR for docs is definitely appreciated!

…

On Sat, Jun 29, 2019 at 4:37 AM Markus Schirp ***@***.***> wrote: I've been bitten by my false assumptions on the behavior of the managerModifyRequest multiple times now. And I always end up reading this thread. Is it okay I send a PR that documents this behavior? Basically I want to say: Its the job of the author of the modification function to ensure idempotency. I think there is no valid use case where the modification function should not be idempotent? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#350?email_source=notifications&email_token=AAAMCB2UJU4ZTHGNITPYOSTP424GTA5CNFSM4E7W2FX2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODY3O6AQ#issuecomment-506916610>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAMCB7HKKKH2DHPK2D2G5LP424GTANCNFSM4E7W2FXQ> .

[rfn01]

What about adding a Data.Vault.Lazy to Request and propagating that vault to the Response? This could enable something akin to Wai's middle-ware. A trial implementation was small and easy. The worse part was losing deriving Show for 'Response`. Of course, this also adds a dependency on the vault package and might not be generally useful - perhaps a cost without a benefit.

My current use case is logging to document SLAs, specifically including the X-Request-ID on the various log messages to tie everything together. I can handle that at a higher level but found the middle-ware approach appealing.

I'm happy to submit a PR if you think such might be useful. I also completely understand if you don't.

[snoyberg]

I'm fairly ambivalent about it. I'm OK with the change if there's no backwards incompatibility introduced.

[rfn01]

I can relate to ambivalence. :-) I'll clean things up, add docs, look at tests, etc..., and if it still looks good, send over a PR. The way things are going, it'll be a few days. Thanks!