make normalize_key cpdef & add warnings for unpickle unmarshal fail by MOON-CLJ · Pull Request #66 · douban/libmc

MOON-CLJ · 2018-01-16T06:03:19Z

warnings.warn for unpickle & unmarshal fail, and only first time for each line of the caller code。

>>> import warnings
>>> warnings.warn("once")
__main__:1: UserWarning: once
>>> warnings.warn("once")
>>> warnings.warn("another")
__main__:1: UserWarning: another
>>> warnings.warn("another")

(venv) chenlijun@redis00 ~/libmc $ python tmp.py
db:foo
tmp.py:7: UserWarning: [libmc] unpickle failed. err:ValueError('insecure string pickle',)
  mc.get('foo')
(venv) chenlijun@redis00 ~/libmc $ cat tmp.py
import libmc
import pickle

mc = libmc.Client(['nain2:11221'], prefix=u'db:')
print mc.normalize_key('foo')
mc.set_raw('foo', pickle.dumps("aaa")[:3], 3600, 1 << 0)
mc.get('foo')

youngsofun · 2018-01-17T10:10:29Z

libmc/_client.pyx

        try:
            dec_val = marshal.loads(dec_val)
        except:
+            warnings.warn("[libmc] unmarshal failed %r" % dec_val)


dec_val 是二进制应该没什么用吧，还比较大

不如把key 打出来？

okay，考虑把key和error都打出来。

@youngsofun 应该不用key，get 知道自己是什么key，get_multi至少知道范围。

youngsofun · 2018-01-17T10:10:51Z

libmc/_client.pyx

                flags[0] = _FLAG_PICKLE
            except:
-                pass
+                warnings.warn("[libmc] marshal & pickle both failed %r" % val)


担心 val 太大

可以先打个 type？

还有 pickle 失败的信息里可能也有有用的信息

mckelvin · 2018-01-17T13:43:00Z

libmc/_client.pyx

-            except:
-                pass
+            except Exception as e:
+                warnings.warn("[libmc] marshal & pickle both failed. type:%r err:%r" % (type_, e))


This may lead to a log flood if the unpackable object is encoded again and again.

@mckelvin #66 (comment) as i explain，it only output at the first time for each msg in warnings.warn(msg)。https://gist.github.com/MOON-CLJ/0f06a1e57d872c6f5f0f9e368326d406 this gist try to make it more clear。

so i think it's enough to avoid log flood in most case。

mckelvin · 2018-01-17T13:44:01Z

libmc/_client.pyx

                alias = PyUnicode_AsUTF8String(alias) if alias else None
            servers_.append((host, port, alias))

-        Py_INCREF(servers_)


Why remove this?

@mckelvin 5968657 i try to explain it in (new) commit msg。

As is described in the doc:

The PyString_AsString return a NUL-terminated representation of the contents of string. The pointer refers to the internal buffer of string, not a copy.

So I think c_hosts[i] is using the host string object, which is inside the servers_.

@mckelvin okay, i'll revert this change.(still not very sure as follow comment)

@mckelvin but i am still not very clear about this line https://github.com/douban/libmc/pull/66/files#diff-721f906017b85cb2e3838d0775a3138dR376

host, port, alias = servers_[i]

host, port, alias is copy or ref?

if it's copy(new variable)，Py_INCREF(severs_) is not necessary。

confirm by @windreamer , it's ref。i'll revert this change。

In [14]: a = 1 In [15]: servers = [[a, 1, 2]] In [16]: _a, _, _ = servers[0] In [17]: id(_a) Out[17]: 24355160 In [18]: id(a) Out[18]: 24355160

@mckelvin

to say the least, servers_ is a func local variable, it's lifetime will expand to the whole func。
so i doubt Py_INCREF(servers_) is just a programming style?

mckelvin · 2018-01-17T13:44:36Z

libmc/_client.pyx

            dec_val = marshal.loads(dec_val)
-        except:
+        except Exception as e:
+            warnings.warn("[libmc] unmarshal failed. err:%r" % e)


Same log flood issue here.

mckelvin · 2018-01-17T13:44:40Z

libmc/_client.pyx

            dec_val = pickle.loads(dec_val)
-        except:
+        except Exception as e:
+            warnings.warn("[libmc] unpickle failed. err:%r" % e)


Same log flood issue here.

mckelvin · 2018-01-17T13:45:40Z

libmc/_client.pyx

        else:
            self._imp.disableConsistentFailover()

-        PyMem_Free(c_hosts)


These 3 lines are moved to earlier lines. Why?

@mckelvin see 5968657 also

because @everpcpc's pr https://github.com/douban/libmc/pull/74/files#diff-721f906017b85cb2e3838d0775a3138dR418 already do this, i'll revert this change。

mckelvin · 2018-01-17T13:49:47Z

I'd appreciate it if you could polish the PR and git commit message: https://chris.beams.io/posts/git-commit/

MOON-CLJ · 2018-01-18T02:45:09Z

I'd appreciate it if you could polish the PR and git commit message: https://chris.beams.io/posts/git-commit/

thx very much , i will try。

this ci include some minor fixes: - c_server_addr to server_addr, because it's basestring not c char array. - c_hosts c_ports c_aliases PyMem_Free after _imp.init immediately， _imp.init dont take over their buffer，_imp.init use snprintf to make copy，so i think free as soon possible do the right thing。

youngsofun · 2018-02-27T03:52:16Z

@MOON-CLJ 从你的 demo看，"首次" 是完全基于log msg 文本的吗？
但如果 pickle 的错误包含 object 内容，或者如果自定义 getstate 就风险：

就可能每次都不同？
除了 log 多，waring模块实现上应该还需要存已经 log 过的（也可能是 hash）？
value print 出来可能会很大

https://github.com/python/cpython/blob/3.6/Lib/pickle.py#L265

所以，可能需要对 err 做裁剪？后者要引用代码证明不存在上述问题）。

另外没有 key，定位可能比较麻烦
当然只有 key 没有具体错误信息，也可能不好定位错误原因
调用栈的信息，可能打出来是有用的？

cython 我写的少，细节如果不放心再找田老师确认一下？

MOON-CLJ · 2018-02-27T05:00:38Z

@youngsofun

从你的 demo看，"首次" 是完全基于log msg 文本的吗？

基本上是，即warnings.warn所在的行数加上warnings.warn的内容去重。https://gist.github.com/MOON-CLJ/0f06a1e57d872c6f5f0f9e368326d406

调用栈的信息，可能打出来是有用的？

https://gist.github.com/MOON-CLJ/0f06a1e57d872c6f5f0f9e368326d406/revisions#diff-1ad944627ac1ed425afb954cf0f6f757L46 从这里判断，对于libmc的情况，出错时，会将调用libmc get方法那一行打印出来，定位也就够了？用法定位到了，也就不用key了？

所以问题应该集中在pickle或者unpickle error的出错信息上？我再确认下。

MOON-CLJ · 2018-02-27T05:08:57Z

对于libmc的情况，出错时，会将调用libmc get方法那一行打印出来，定位也就够了？用法定位到了，也就不用key了？

不过对于cache deco这种集中调用get的地方不适用。

MOON-CLJ · 2018-02-27T06:16:28Z

如果自定义 getstate 就风险

验证了下，确实会有风险。

cat /tmp/test_pickle.py
import pickle


class C(object):
    def __getstate__(self):
        #raise Exception("aaaa")
        return True

    def __setstate__(self, state):
        raise Exception("aaaa")
        pass


c = C()
print c
cc = pickle.dumps(c)
print [cc]
c = pickle.loads(cc)
print c

MOON-CLJ · 2018-02-27T06:40:26Z

@youngsofun https://gist.github.com/MOON-CLJ/0f06a1e57d872c6f5f0f9e368326d406 gist updated

how about just print value type and stack?

youngsofun · 2018-02-27T07:11:27Z

@MOON-CLJ 感觉， libmc 内部调用 decode 位置固定， key 应该可以在上一两层的 python frame 对象中找到？和 stack 一样单独 log 出来即可。先要确定不是外部的调用

MOON-CLJ · 2018-02-27T07:27:14Z

@youngsofun https://gist.github.com/MOON-CLJ/0f06a1e57d872c6f5f0f9e368326d406#file-python-warnings-warn-test-L75 将拿key的逻辑写在这个function里感觉也有点奇怪。

youngsofun · 2018-02-27T07:33:14Z

@MOON-CLJ 可以先这样上，遇到具体问题再说

youngsofun · 2018-02-27T09:10:47Z

直接改 warnings，似乎不妥？其他人也用呢？

MOON-CLJ · 2018-02-27T09:12:01Z

@youngsofun 嗯也是哈。我再想想。

warning about pickle and marshal err when encode_value or decode_value

MOON-CLJ · 2018-02-27T09:46:57Z

@youngsofun updated

ChangeLog: - use pickle protocol version 2 (#52) - fix rvalue reference (#53) - Fix tests (#55) - golibmc_test: Fix fragile test case (#56) - Use connection pool in golibmc(again) (#57) - golibmc Quit ret err (#59) - cmake: Incorporate gtest in a standard way (#58) - Rename refactor (#60) - sync rapidjson/itoa.h upstream fix (#65) - split cppcheck (#67) - split FSM_GET_BYTES_CAS (#68) - Avoid noisy recv_err when broadcast quit (#73) - Try to support updating some servers without affecting others (#74) - make normalize_key cpdef & add warnings for unpickle unmarshal fail (#66) - fix fcntl usage when set socket nonblock (#69) - minor fix (#61) - add condition log macro (#77) - Define and use notWaitForRetryTimeout (#82) - upgrade travis to gcc 7 (#91) - add func errCodeToString (#79) - Introduce a reconnect mechanism in waitPoll (#88)

MOON-CLJ changed the title ~~make normalize_key cpdef & add warnings for unpickle unmarshal fail~~ make normalize_key cpdef & add warnings for unpickle unmarshal fail[WIP] Jan 16, 2018

MOON-CLJ force-pushed the cython_minor_fix branch 2 times, most recently from 4abf267 to aeeb318 Compare January 16, 2018 07:07

MOON-CLJ changed the title ~~make normalize_key cpdef & add warnings for unpickle unmarshal fail[WIP]~~ make normalize_key cpdef & add warnings for unpickle unmarshal fail Jan 17, 2018

MOON-CLJ force-pushed the cython_minor_fix branch 2 times, most recently from 0a82963 to c1a0626 Compare January 17, 2018 09:07

youngsofun reviewed Jan 17, 2018

View reviewed changes

MOON-CLJ force-pushed the cython_minor_fix branch from c1a0626 to 5879666 Compare January 17, 2018 10:36

mckelvin suggested changes Jan 17, 2018

View reviewed changes

MOON-CLJ force-pushed the cython_minor_fix branch from 5879666 to 08b7690 Compare January 18, 2018 03:52

MOON-CLJ force-pushed the cython_minor_fix branch 4 times, most recently from 1010f18 to c9cb4d7 Compare January 30, 2018 08:47

MOON-CLJ force-pushed the cython_minor_fix branch from c9cb4d7 to 871dd04 Compare February 26, 2018 03:50

MOON-CLJ added 2 commits February 26, 2018 08:40

let python code can use normalize_key

8573efb

MOON-CLJ force-pushed the cython_minor_fix branch from 871dd04 to cc83b57 Compare February 26, 2018 08:41

MOON-CLJ force-pushed the cython_minor_fix branch from cc83b57 to 86ca251 Compare February 27, 2018 07:48

let user have a chance to know cython encode or decode err

583e147

warning about pickle and marshal err when encode_value or decode_value

MOON-CLJ force-pushed the cython_minor_fix branch from d16eac9 to 583e147 Compare February 27, 2018 09:46

youngsofun approved these changes Feb 27, 2018

View reviewed changes

MOON-CLJ merged commit 8317ccd into douban:master Feb 27, 2018

Conversation

MOON-CLJ commented Jan 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youngsofun Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MOON-CLJ Jan 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MOON-CLJ Jan 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MOON-CLJ Jan 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mckelvin commented Jan 17, 2018

Uh oh!

MOON-CLJ commented Jan 18, 2018

Uh oh!

youngsofun commented Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MOON-CLJ commented Feb 27, 2018

Uh oh!

MOON-CLJ commented Feb 27, 2018

Uh oh!

MOON-CLJ commented Feb 27, 2018

Uh oh!

MOON-CLJ commented Feb 27, 2018

Uh oh!

youngsofun commented Feb 27, 2018

Uh oh!

MOON-CLJ commented Feb 27, 2018

Uh oh!

youngsofun commented Feb 27, 2018

Uh oh!

youngsofun commented Feb 27, 2018

Uh oh!

MOON-CLJ commented Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MOON-CLJ commented Jan 16, 2018 •

edited

Loading

youngsofun Jan 17, 2018 •

edited

Loading

MOON-CLJ Jan 29, 2018 •

edited

Loading

MOON-CLJ Jan 29, 2018 •

edited

Loading

MOON-CLJ Jan 29, 2018 •

edited

Loading

youngsofun commented Feb 27, 2018 •

edited

Loading

MOON-CLJ commented Feb 27, 2018 •

edited

Loading