Opened 16 years ago

Closed 15 years ago

#1514 defect closed fixed (fixed)

twisted.spread.banana check for INT vs. LONGINT will fail on some 64-bit platforms

Reported by: itamarst Owned by:
Priority: highest Milestone:
Component: pb Keywords:
Cc: Branch:
Author:

Description

The code checks using isinstance(o, int), but the protocol says LONGINT is for >32bit, and ints are 64-bit on some platforms.

Attachments (1)

int64.diff (2.5 KB) - added by Glyph 16 years ago.

Download all attachments as: .zip

Change History (20)

Changed 16 years ago by Glyph

Attachment: int64.diff added

comment:1 Changed 16 years ago by Glyph

Component: corepb
Keywords: review added
Milestone: Twisted-2.5
Owner: changed from Glyph to itamarst
Priority: normalhighest

comment:2 Changed 16 years ago by Glyph

I think this patch should fix your issue.

comment:3 Changed 15 years ago by jknight

I don't like testSizedIntegerTypes, because long/int keep getting closer in python, I wouldn't be surprised if long(1) returned an int in python 2.6.

comment:4 Changed 15 years ago by Stephen Thorne

Keywords: review removed
Owner: changed from itamarst to Glyph

I don't consider python2.6 compatibility a blocker to merging a bugfix for the versions of python that actually exist. This patch exists, it passes the tests that exist, and we have buildbot for the day when python2.6alpha is released.

Please merge this change.

comment:5 Changed 15 years ago by Glyph

Resolution: fixed
Status: newclosed

(In [17652]) Allow large numbers to be encoded and decoded by banana on 64-bit hosts.

Fixes #1514

Author: glyph

Reviewer: itamarst

This fixes a long-standing 64-bit bug in banana.

comment:6 Changed 15 years ago by Glyph

James, FWIW, I agree that the test is kind of lame, but at least it provides some coverage. When the 64-bit buildslave is around more reliably I think we'll probably have more attention devoted to correctness in this area.

comment:7 Changed 15 years ago by Jean-Paul Calderone

(In [17751]) Revert r17652 - test failures

Refs #1514

The new test added in this revision fails with Canana on 64 bit platforms. Canana should probably just be deleted altogether.

comment:8 Changed 15 years ago by Jean-Paul Calderone

Resolution: fixed
Status: closedreopened

comment:9 Changed 15 years ago by Glyph

Probably. Is it even built by default any more?

comment:10 Changed 15 years ago by Glyph

Owner: changed from Glyph to jknight
Status: reopenednew

Seeing that I don't actually have any 64bit OSes any more, I'm probably not the right person to test this and re-merge.

comment:11 Changed 15 years ago by jknight

Test what? I don't see anything changed?

comment:12 Changed 15 years ago by Jean-Paul Calderone

Presumably, test some Canana changes that no one has tried to make yet?

comment:13 Changed 15 years ago by Glyph

To clarify: canana should be removed, and the fix (which has been reverted) be reinstated. I can do both of these things, but the only mechanism I have to test is running via the buildbot, which is a pain.

comment:14 Changed 15 years ago by radix

Milestone: Twisted-2.5

UNPRIORITIZED (as release critical)

comment:15 Changed 15 years ago by Jean-Paul Calderone

Hey look! Canana is gone.

comment:16 Changed 15 years ago by Jean-Paul Calderone

Keywords: review added
Owner: jknight deleted

I tried applying the patch but it conflicted in both places. As I was resolving the conflicts in the test module I noticed the test was somewhat incomplete. I rewrite it to cover several values on each side of each boundary. As I was about to resolve the conflict in banana.py I noticed the problem had already been fixed by another recent banana branch.

So this is just test changes now, in banana-longint-1514

comment:17 Changed 15 years ago by jknight

Keywords: review removed
Owner: set to Jean-Paul Calderone

Looks good. Test passes on both x86-64 and PPC-32, so I concur that the problem was fixed.

comment:18 Changed 15 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [19179]) Merge banana-longint-1514

Author: exarkun Reviewer: jknight Fixes #1514

Add test coverage for the behavior of banana int serialization near the boundary between small integer tokens and large integer tokens, for both positive and negatives. No actual fixes to banana, since they seem to have been made already.

comment:19 Changed 11 years ago by <automation>

Owner: Jean-Paul Calderone deleted
Note: See TracTickets for help on using tickets.