search

docker / libchan

Like Go channels over the network
Apache License 2.0
2.47k stars 142 forks source link

Support encoding.BinaryMarshaler types #75

Open stevvooe opened 9 years ago

stevvooe commented 9 years ago

Sending time.Time values over libchan has resulted in panics. This is a core type and correct handling of the time type in libchan is important. By adding support for types that implement encoding.BinaryMarshaler, we get support for time.Time along with other types that implement this interface.

At first, it was unclear why this correctly worked, but closer examination showed that this is supported within the codec package. The main bug was in the handling in the (*pipeReceiver).copyValue method and its spdy equivalent. The value copier really needs to be generalized and ported to work with any libchan implementation to avoid such bugs in the future.

stevvooe commented 9 years ago

@dmcgowan It might be better to use TextMarshaler for the time type or define a msgpack extension type for this functionality.

Note that this is blocking ipc support in v2 registry: docker/docker-registry#831.

mcollina commented 9 years ago

I'm in for an extension type for times. Il giorno lun 8 dic 2014 alle 22:32 Stephen Day notifications@github.com ha scritto:

@dmcgowan https://github.com/dmcgowan It might be better to use TextMarshaler for the time type or define a msgpack extension type for this functionality.

Note that this is blocking ipc support in v2 registry: docker/docker-registry#831 https://github.com/docker/docker-registry/issues/831.

— Reply to this email directly or view it on GitHub https://github.com/docker/libchan/pull/75#issuecomment-66191230.

dmcgowan commented 9 years ago

+1 on defining an extended type for time. @stevvooe is this currently blocking any implementation or is using string a good enough work around for now? If it is we should discuss what the extended type definition is and get a PR to the spec and implementation.

stevvooe commented 9 years ago

@dmcgowan If we change this diff to use TextMarshaler instead, I think it will meet the needs for now. Would you like me to file a ticket for "native" time support?

stevvooe commented 9 years ago

What was the conclusion here? Did we want to just use TextMarshaler?

docker/distribution#28 is pending a resolution here.

dmcgowan commented 9 years ago

The code updated in this PR will be deprecated by #79. Do you have a timeframe for needing this feature? I will close it when the other is ready and merged. If you need it sooner we can discuss merging, I just want to ensure this doesn't require doing something special by the caller.

stevvooe commented 9 years ago

I was just cleaning house in docker/distribution. I can link docker/distribution#28 #79, instead.