Message95019
> rlock_acquire_doc: "(...) return None once the lock is acquired".
> That's wrong, acquire() always return a boolean (True or False).
You're right, I should fix that docstring.
> rlock_release(): Is the assert(self->rlock_count > 0); really required?
> You're checking its value few lines before.
Right again :) I forgot this was unsigned.
> rlock_acquire_restore(): raise an error if the lock acquire failed,
> whereas rlock_acquire() doesn't. Why not raising an error in
> rlock_acquire() (especially if blocking is True)?
For rlock_acquire(), I mimicked what the Python code (_PyRLock.acquire)
does: if locking fails, it returns False instead. It is part of the API.
(and I agree this is not necessarily right, because failing to lock if
blocking is True would probably indicate a low-level system error, but
the purpose of the patch is not to change the API)
But you're right that the Python version of rlock_acquire_restore()
doesn't check the return code either, so I may remove this check from
the C code, although the rest of the method clearly assumes the lock has
been taken.
> rlock_acquire_restore(): (maybe) set owner to 0 if count is 0.
>
> rlock_release_save(): may also reset self->rlock_owner to 0, as
> rlock_release().
As I said to Gregory, the current code doesn't rely on rlock_owner to be
reset but, yes, we could still add it out of safety.
> rlock_new(): why not reusing type instead of Py_TYPE(self) in
> "Py_TYPE(self)->tp_free(self)"?
Good point.
> __exit__: rlock_release() is defined as __exit__() with METH_VARARGS,
> but it has no argument. Can it be a problem?
I just mimicked the corresponding declarations in the non-recursive lock
object. Apparently it's safe on all architectures Python has been
running on for years, although I agree it looks strange.
> If your patch is commited, you can reconsider #3618 (possible deadlock
> in python IO implementation).
Indeed.
Thanks ! |
|
Date |
User |
Action |
Args |
2009-11-07 15:17:16 | pitrou | set | recipients:
+ pitrou, gregory.p.smith, jcea, Rhamphoryncus, hgibson50, vstinner, giampaolo.rodola, kevinwatters, sserrano |
2009-11-07 15:17:14 | pitrou | link | issue3001 messages |
2009-11-07 15:17:13 | pitrou | create | |
|