Update to network 3.0 with backward compatibility for network <= 2.8 #98

scott-fleischman · 2019-05-23T19:24:31Z

This PR is more of a request for comments than a PR intended to be merged right away. It uses network 3 and network-bsd (using this PR on bson). The build succeeds and tests pass with stack. I added a stack.yaml file and made other choices that I expect you would like to do differently before actually merging.

How would you like to move forward in updating this package to network 3? If you would like to keep compatibility with older network versions, would you like to do that via CPP or some other method? Is breaking backward compatibility a possible option?

See Issue #95

scott-fleischman · 2019-05-23T20:47:30Z

Benchmark comparison

master

benchmarking insert/1000
time                 93.61 ms   (83.17 ms .. 102.2 ms)
                     0.975 R²   (0.905 R² .. 0.999 R²)
mean                 93.53 ms   (90.32 ms .. 100.2 ms)
std dev              7.229 ms   (3.543 ms .. 11.79 ms)
variance introduced by outliers: 21% (moderately inflated)

PR

benchmarking insert/1000
time                 92.81 ms   (87.57 ms .. 106.6 ms)
                     0.972 R²   (0.890 R² .. 1.000 R²)
mean                 94.75 ms   (91.74 ms .. 102.3 ms)
std dev              7.330 ms   (3.163 ms .. 11.47 ms)
variance introduced by outliers: 21% (moderately inflated)

VictorDenisov · 2019-05-26T22:33:50Z

Abandoning network 2.8 will inconvenience us and the community. If we abandon 2.8 and publish a major version bump we will need to support two codebases and port fixes from newer version to another. Stackage nightly builds are still using network 2.8. I would suggest to introduce conditional compilation here. One of breaking changes that we can afford is to remove exports of primitives imported from Network and replace with our own.

scott-fleischman · 2019-05-27T18:37:37Z

Ok so I understand that you want to keep backward compatibility.

One of breaking changes that we can afford is to remove exports of primitives imported from Network and replace with our own.

Ok so I will update this PR with an approach that adds a Mongo-specific PortId type which internally uses CPP for the different network versions.

Would you like to copy the connectTo code from the old Network module too? Or would you prefer to propagate changes that use Network.Socket more directly?

VictorDenisov · 2019-05-28T03:40:19Z

I think we need to hid Network.Socket as well. Otherwise the client of the library will need to do conditional compilation as well.

Due to this warning: "This ciphersuite list contains RC4. Use ciphersuite_strong or ciphersuite_default instead."

Due to the following warning: "Module ‘Control.Monad.Error’ is deprecated: Use "Control.Monad.Except" instead"

Due to deprecations: "Deprecated: Use ConduitT directly".

Due to warning: [-Wunused-top-binds].

This reverts commit 5f04dc6. Leave the use of Producer and Consumer for now until we drop support for conduit-1.2.*. conduit-1.3 introduces ConduitT and deprecates the use of type synonyms. However, ConduitT is not present in conduit-1.2.

scott-fleischman · 2019-05-30T00:23:38Z

Related: commercialhaskell/stackage#4528

Add flag imitating bson package PR for network changes. Add stack files for compilation checking. Both ghc86 builds work. Still need to fix ghc84 and under builds with older network code.

scott-fleischman · 2019-05-30T01:04:01Z

@VictorDenisov let me know if you like the direction in 21cf023 . I still need to fix a few things for older network versions.

Also I'm not sure why the build is failing. It appears to be due to things not related to the code changes. Perhaps the CI script needs updating?

VictorDenisov · 2019-05-30T01:09:11Z

Yep. The direction looks right to me. I'll take a look at the build scripts this weekend. Looks like mongodb deprecated some of their older versions.

scott-fleischman · 2019-05-30T18:05:19Z

@VictorDenisov ok the tests pass across the various stack files, and the benchmarks run, all in the same ballpark.

In particular this allows you to use `fromIntegral` without having to add the newtype wrapper. This can help existing code move away from importing and referencing the PortID type altogether.

scott-fleischman · 2019-05-30T18:50:27Z

Let me know what else you would like to do for this PR to finalize it for merging. It would probably be good to merge the corresponding bson PR at the same time.

VictorDenisov

@scott-fleischman good PR. Thanks for splitting it into good easy to read commits. I have one question before merging the change.

VictorDenisov · 2019-06-03T02:29:37Z

mongoDB.cabal

@@ -54,14 +60,22 @@ Library
                    , base64-bytestring >= 1.0.0.1
                    , nonce >= 1.0.5

+  if flag(_old-network)


@scott-fleischman Can you clarify for me who and how is supposed to set this flag?

They are set using the cabal configuration flags as described here.

There are a couple choices involved here that I basically deferred to the choices made in the bson PR.

The name of the flag. _old-network is pretty cryptic. I just copied it. If flags can contain numbers it might be better to name it something like use-network2 or something like that.

Default value. Again I made it False by default but maybe until network-3.* gets into stackage, maybe the default should be true? Then when stackage includes network 3 by default in the newest stackage lts then mongoDB could ship a breaking change which changes the flag to true, so it defaults to using network3.

I don't believe the .cabal files themselves allow logic like conditional statements based on a library dependency version. In our case we would need to include a new library (network-bsd) if network is >=3.0, so that might not be possible without configure flags.

In the end, I am happy to consider alternatives to the approach taken in this PR with regards to cabal configuration. But as far as I can tell that is going to involve some user choice of which network to use.

You can see how I set them in the stack files, for example:
https://github.com/plow-technologies/mongodb/blob/f84cc035179198ed63a5e247aa7c95f61ce7a295/stack-ghc80.yaml#L2-L4

The flag is omitted here, so it defaults to using the network 3.0+ and network-bsd dependencies.
https://github.com/plow-technologies/mongodb/blob/network-deprecations/stack-ghc86-network3.yaml

Maybe manual: False might be useful here though and would work out-of-the-box in both cases (network 2 or network 3). https://www.haskell.org/cabal/users-guide/developing-packages.html#pkg-field-flag-manual

By default, Cabal will first try to satisfy dependencies with the default flag value and then, if that is not possible, with the negated value. However, if the flag is manual, then the default value (which can be overridden by commandline flags) will be used.

It doesn't seem to work for stack if I remove the explicit flag set. I get errors in the stack configure step.

cabal (cabal-install version 2.4.0.0) itself seems ok with it though. I was able to build using ghc 8.2.2 and it picked the old network configurations even though the _old-network flag defaults to False.

So all in all, it seems to me the current approach seems pretty good given the various options I am aware of.

Stack users will have to set the flag explicitly for network <=2.8.
Cabal users should work fine as-is. (I'm not sure how to test against the newer network because I'm not sure how to get cabal to know about the bson PR, which it would need to for network3.)

So with defaulting the flag to false, stack users of network 2.8 will have to explicitly set it to true (or explicitly include network 3). When the stackage lts includes network 3, then can remove the explicit flag from their configuration.

Cabal users should work fine in both cases. When network 3 is available for their configuration (needs bson available that works with network 3), since the _old-network flag default to True, it will try the network 3 config first, so it will use that. Else it default to the old network configuration and builds fine. I tested with ghc 8.2.2 and ghc 8.6.5 with cabal 2.4.

Here's a reference to the conditional options in a cabal file: https://www.haskell.org/cabal/users-guide/developing-packages.html#conditions

scott-fleischman · 2019-06-11T16:36:48Z

@VictorDenisov any further thoughts or changes you would like to see?

VictorDenisov · 2019-06-12T03:05:14Z

Sorry, I was busy a bit. Looking into it now.

VictorDenisov · 2019-06-12T03:14:33Z

I suggest to set this flag default to True because stackage build system doesn't know how to set this flag. We need to be compatible with the current stackage by default. If default build starts failing mongodb package will be taken out of nightly builds.

VictorDenisov · 2019-06-12T03:17:24Z

Do we need to merge bson PR first?

Following mongodb-haskell/bson@69378a0

scott-fleischman · 2019-06-12T15:58:36Z

Yeah best to merge bson PR first. I removed the default setting for the flag following the bson commit.

scott-fleischman added 7 commits May 29, 2019 16:30

Add .stack-work to .gitignore.

4477045

Use ciphersuite_default instead of ciphersuite_all.

13f56bb

Due to this warning: "This ciphersuite list contains RC4. Use ciphersuite_strong or ciphersuite_default instead."

Use Control.Monad.Except instead of Control.Monad.Error.

c03e1ed

Due to the following warning: "Module ‘Control.Monad.Error’ is deprecated: Use "Control.Monad.Except" instead"

Remove use of conduit Producer and Consumer.

5f04dc6

Due to deprecations: "Deprecated: Use ConduitT directly".

Prefix internal unused fields with underscore.

b094dff

Due to warning: [-Wunused-top-binds].

Update shadowing warnings.

74a4041

scott-fleischman force-pushed the network-deprecations branch from c819606 to 5bb7751 Compare May 30, 2019 00:20

Add module Database.MongoDB.Internal.Network.

21cf023

Add flag imitating bson package PR for network changes. Add stack files for compilation checking. Both ghc86 builds work. Still need to fix ghc84 and under builds with older network code.

scott-fleischman added 3 commits May 30, 2019 10:02

Use network's connectTo when available.

17287b5

Use same version check as in cabal file; fix typo in comment.

996d3e1

Improve network versioning; add to benchmarks.

3334d81

Add numeric instances that network's PortNumber has.

f84cc03

In particular this allows you to use `fromIntegral` without having to add the newtype wrapper. This can help existing code move away from importing and referencing the PortID type altogether.

scott-fleischman changed the title ~~RFC: Update to network 3.0 and use Control.Monad.Except.~~ Update to network 3.0 with backward compatibility for network <= 2.8 May 30, 2019

VictorDenisov reviewed Jun 3, 2019

View reviewed changes

Remove explicit default of _old-network flag.

ef1fc38

Following mongodb-haskell/bson@69378a0

VictorDenisov merged commit ef1fc38 into mongodb-haskell:master Jun 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to network 3.0 with backward compatibility for network <= 2.8 #98

Update to network 3.0 with backward compatibility for network <= 2.8 #98

scott-fleischman commented May 23, 2019

scott-fleischman commented May 23, 2019

VictorDenisov commented May 26, 2019

scott-fleischman commented May 27, 2019

VictorDenisov commented May 28, 2019

scott-fleischman commented May 30, 2019

scott-fleischman commented May 30, 2019

VictorDenisov commented May 30, 2019

scott-fleischman commented May 30, 2019

scott-fleischman commented May 30, 2019 •

edited

Loading

VictorDenisov left a comment

VictorDenisov Jun 3, 2019

scott-fleischman Jun 3, 2019

scott-fleischman Jun 3, 2019 •

edited

Loading

scott-fleischman Jun 3, 2019 •

edited

Loading

scott-fleischman Jun 3, 2019 •

edited

Loading

scott-fleischman Jun 3, 2019

scott-fleischman commented Jun 11, 2019

VictorDenisov commented Jun 12, 2019

VictorDenisov commented Jun 12, 2019

VictorDenisov commented Jun 12, 2019

scott-fleischman commented Jun 12, 2019

Update to network 3.0 with backward compatibility for network <= 2.8 #98

Update to network 3.0 with backward compatibility for network <= 2.8 #98

Conversation

scott-fleischman commented May 23, 2019

scott-fleischman commented May 23, 2019

Benchmark comparison

master

PR

VictorDenisov commented May 26, 2019

scott-fleischman commented May 27, 2019

VictorDenisov commented May 28, 2019

scott-fleischman commented May 30, 2019

scott-fleischman commented May 30, 2019

VictorDenisov commented May 30, 2019

scott-fleischman commented May 30, 2019

scott-fleischman commented May 30, 2019 • edited Loading

VictorDenisov left a comment

Choose a reason for hiding this comment

VictorDenisov Jun 3, 2019

Choose a reason for hiding this comment

scott-fleischman Jun 3, 2019

Choose a reason for hiding this comment

scott-fleischman Jun 3, 2019 • edited Loading

Choose a reason for hiding this comment

scott-fleischman Jun 3, 2019 • edited Loading

Choose a reason for hiding this comment

scott-fleischman Jun 3, 2019 • edited Loading

Choose a reason for hiding this comment

scott-fleischman Jun 3, 2019

Choose a reason for hiding this comment

scott-fleischman commented Jun 11, 2019

VictorDenisov commented Jun 12, 2019

VictorDenisov commented Jun 12, 2019

VictorDenisov commented Jun 12, 2019

scott-fleischman commented Jun 12, 2019

scott-fleischman commented May 30, 2019 •

edited

Loading

scott-fleischman Jun 3, 2019 •

edited

Loading

scott-fleischman Jun 3, 2019 •

edited

Loading

scott-fleischman Jun 3, 2019 •

edited

Loading