Getting the error "Always is not a Monoid"

[jacobstanley]

This isn't necessarily a bug report, but I'm getting the error "Always is not a Monoid" while trying to decode a message. Do you have any pointers on how I can debug this?

If it helps I can post the code and an example set of bytes which replicates the problem.

[NathanHowell]

A small example which reproduces this error would be helpful. It could be a bug in the library, the Always Monoid instance is a bit of a hack.

[jacobstanley]

So I wouldn't call it a small example (sorry!) but it is completely self contained.

If you clone https://github.com/jystic/protobuf-always-monoid-repro then build and run you should see the error.

If you comment out this optional field, the error goes away.

[jacobstanley]

If you're wondering what the code relates to, I'm experimenting with talking to Hadoop using it's native TCP interface - first step is a native Haskell client for HDFS. Thought you might be interested after looking at the other repos for alphaHeavy.

[nilscc]

This error occurs when a Required field is missing iirc. Pretty sure the Nothing -> pure . K1 . Field $ c mempty case of fieldDecode in the Decode.hs is to blame here.

[jacobstanley]

Cool, if that's the case debugging it will be easy, will report back soon.

If it turns out to be true, perhaps we should change the error to "Required field missing" or similar.

[jacobstanley]

Ok, wasn't as easy as I thought, commenting out various Required fields doesn't help isolating the problem.

[NathanHowell]

Here's the stack:

GetListingResponse {glDirList = Field {runField = Optional {runOptional = Just (Message {runMessage = DirectoryListing {dlPartialListing = Field {runField = Repeated {runRepeated = [Message {runMessage = FileStatus {fsFileType = Field {runField = Required {runRequired = Always {runAlways = Enumeration {runEnumeration = Dir}}}}, fsPath = Field {runField = Required {runRequired = Always {runAlways = Value {runValue = "hbase"}}}}, fsLength = Field {runField = Required {runRequired = Always {runAlways = Value {runValue = 0}}}}, fsPermission = Field {runField = Required {runRequired = Always {runAlways = Message {runMessage = FilePermission {fpPerm = Field {runField = Required {runRequired = Always {runAlways = Value {runValue = 493}}}}}}}}}, fsOwner = Field {runField = Required {runRequired = Always {runAlways = Value {runValue = "hbase"}}}}, fsGroup = Field {runField = Required {runRequired = Always {runAlways = Value {runValue = "hbase"}}}}, fsModificationTime = Field {runField = Required {runRequired = Always {runAlways = Value {runValue = 1394466373004}}}}, fsAccessTime = Field {runField = Required {runRequired = Always {runAlways = Value {runValue = 0}}}}, fsSymLink = Field {runField = Optional {runOptional = Last {getLast = Nothing}}}, fsBlockReplication = Field {runField = Optional {runOptional = Last {getLast = Just (Value {runValue = 0})}}}, fsBlockSize = Field {runField = Optional {runOptional = Last {getLast = Just (Value {runValue = 0})}}}, fsLocations = Field {runField = Optional {runOptional = Just (Message {runMessage = LocatedBlocks {lbFileLength = Field {runField = Required {runRequired = Always {runAlways = Value {runValue = Stopped at <exception thrown>
_exception :: e = _
[<exception thrown>] *Main> :force _exception
_exception = GHC.Exception.SomeException "Always is not a Monoid"
[<exception thrown>] *Main> :history
-1  : mempty (Data/ProtocolBuffers/Types.hs:78:12-41)
-2  : gmempty (Data/ProtocolBuffers/Message.hs:125:13-21)
-3  : gmempty (Data/ProtocolBuffers/Message.hs:113:13-22)
-4  : showsPrec (Data/ProtocolBuffers/Types.hs:41:72-75)
-5  : showsPrec (Data/ProtocolBuffers/Types.hs:75:64-67)
-6  : showsPrec (Data/ProtocolBuffers/Types.hs:47:72-75)
-7  : showsPrec (Data/ProtocolBuffers/Types.hs:67:72-75)
-8  : gmempty (Data/ProtocolBuffers/Message.hs:117:13-31)
-9  : gmempty (Data/ProtocolBuffers/Message.hs:117:13-31)
-10 : gmempty (Data/ProtocolBuffers/Message.hs:117:13-31)
-11 : gmempty (Data/ProtocolBuffers/Message.hs:117:13-31)
-12 : gmempty (Data/ProtocolBuffers/Message.hs:113:13-22)
-13 : gmempty (Data/ProtocolBuffers/Message.hs:113:13-22)
-14 : mempty (Data/ProtocolBuffers/Message.hs:99:12-23)
-15 : mempty (Data/ProtocolBuffers/Message.hs:99:12-33)
-16 : showsPrec (Main.hs:115:26-29)
-17 : showsPrec (Data/ProtocolBuffers/Message.hs:96:41-44)
-18 : gdecode (Data/ProtocolBuffers/Message.hs:106:30-44)
-19 : fieldDecode (Data/ProtocolBuffers/Decode.hs:92:37-44)
-20 : fieldDecode (Data/ProtocolBuffers/Decode.hs:92:24-33)
...

[jacobstanley]

It's odd because lbFileLength is a required field, and using the protocol-buffers-fork library it decodes successfully.

The relevant code from the original protobuf is this:

message GetListingResponse {
  optional DirectoryListing dirList = 1;
}

message DirectoryListing {
  repeated HdfsFileStatus partialListing = 1;
  required uint32 remainingEntries  = 2;
}

message HdfsFileStatus {
  enum FileType {
    IS_DIR = 1;
    IS_FILE = 2;
    IS_SYMLINK = 3;
  }
  required FileType fileType = 1;
  required bytes path = 2;          // local name of inode encoded java UTF8
  required uint64 length = 3;
  required FsPermission permission = 4;
  required string owner = 5;
  required string group = 6;
  required uint64 modification_time = 7;
  required uint64 access_time = 8;

  // Optional fields for symlink
  optional bytes symlink = 9;             // if symlink, target encoded java UTF8 

  // Optional fields for file
  optional uint32 block_replication = 10 [default = 0]; // only 16bits used
  optional uint64 blocksize = 11 [default = 0];
  optional LocatedBlocks locations = 12;  // suppled only if asked by client
} 

message FsPermission {
  required uint32 perm = 1;       // Actually a short - only 16bits used
}

message LocatedBlocks {
  required uint64 fileLength = 1;
  repeated LocatedBlock blocks = 2;
  required bool underConstruction = 3;
  optional LocatedBlock lastBlock = 4;
  required bool isLastBlockComplete = 5;
}

// snip

I can post the rest of the dependant structures (i.e. LocatedBlock, etc) if necessary, but they add a lot of extra detail which probably isn't relevant.

[NathanHowell]

The error is happening because fsLocations = Just ... when it should be None. The protobuf binary file doesn't include any values for field 12.

[jacobstanley]

Oh right, of course, so does it seem to be a deserialization bug in protobuf?

[NathanHowell]

Yep, something is amiss. It can be worked around by changing the required fields to be optional.. I'll see if I can track down the problem, brain is a little shot tho (on my way to ICFP).

[jacobstanley]

Oh cool, I'm there right now hacking on this :)

[NathanHowell]

Nice, I'll be there tomorrow :-)

[NathanHowell]

@mcmaniac is probably right. Changing the Nothing case in fieldDecode might be the fix. I haven't done much validation but at least this test case doesn't fail:

fieldDecode
  :: forall a b i n p . (DecodeWire a, Monoid a, KnownNat n)
  => (a -> b)
  -> HashMap Tag [WireField]
  -> Get (K1 i (Field n b) p)
{-# INLINE fieldDecode #-}
fieldDecode c msg =
  let tag = fromIntegral $ natVal (Proxy :: Proxy n)
  in case HashMap.lookup tag msg of
    Just val -> K1 . Field . c <$> foldMapM decodeWire val
    Nothing  -> empty

[NathanHowell]

Can you try out HEAD?

[jacobstanley]

Just tried it, works perfectly now, thanks!

[NathanHowell]

Sweet. I'll cut a release after I get some tests added, probably mid week.

[NathanHowell]

I've added new QuickCheck properties that exercise this bug in 692ff3efdd7e6d7bf7143a3be1edb21c58c1600b, and released the fix to Hackage as 0.2.0.4.

[jacobstanley]

Hey would be great to catch up at ICFP, have some things I'd like to talk to you about regarding performance improvements to the 'binary' package.

[NathanHowell]

I'm in the Haskell symposium today, wearing a shirt with green and orange triangles on the front. Definitely interested in hearing your ideas. On Sep 4, 2014 9:00 AM, "Jacob Stanley" notifications@github.com wrote:

Hey would be great to catch up at ICFP, have some things I'd like to talk to you about regarding the performance improvements to the 'binary' package.

— Reply to this email directly or view it on GitHub https://github.com/alphaHeavy/protobuf/issues/13#issuecomment-54418222.