Conversation
* Fixes `TypeError: exception class/object expected` on truffleruby because #raise with a non-Exception non-String is not supported currently on TruffleRuby.
| def guard_response_too_large! = (raise self if response_too_large?) | ||
| def guard_response_too_large! = (raise exception if response_too_large?) | ||
|
|
||
| def exception(msg = nil) |
There was a problem hiding this comment.
I don't know the intention but it sounds unusual to implement #exception on non-Exception objects.
It means e.g. users could do raise someNet_IMAP_ResponseReader_instance but I would think very few would expect to work, they would expect an error because it's raising something which isn't an Exception.
So maybe this method should be inlined in its only caller, like before #642?
There was a problem hiding this comment.
Yeah, I was getting way too cute with that one! 🤣 I just did it that way in passing, without thinking very much of it, and I'd completely intended to replace it before merging! I do like the idea of relying on #exception duck-typing, where it makes sense semantically. But... it does not make semantic sense here.
There was a problem hiding this comment.
Another refactoring branch (with a more substantial change to mitigate against pathological performance issues) was raising the same exception from multiple places. I extracted that and named it #exception as a temporary placeholder. But I discovered a more significant performance improvement at the same time. So it accidentally migrated into into this branch after a few rebase/cherry-pick rearrangements. 😳
It's only called from one place. And `#exception` isn't the right name for that method anyway.
nevans
force-pushed
the
raise-pass-exception
branch
from
eb01239 to
57f3cda
Compare
nevans
left a comment
nevans
left a comment
There was a problem hiding this comment.
I actually ran the tests before pushing, this time. 😳
Hopefully truffleruby will pass with this?
|
I wasn't triggering that timeout locally, but since I now know about |
|
Hmm.. no. I think this may actually be due to 0e89dbb! |
This test was broken by 0e89dbb, which changed the semantics of when `max_response_size` takes effect. But it's a race condition, so TruffleRuby was hitting it frequently, and I hadn't seen it in CRuby yet.
nevans
force-pushed
the
raise-pass-exception
branch
from
d993bfa to
236832f
Compare
|
Yes, it was due to 0e89dbb. I'd forgotten that I'd added a test for the original behavior. And now I'm second-guessing whether I want that behavior change to go out in an |
|
Great, thank you for the fixes! |
2 participants
TypeError: exception class/object expectedon truffleruby because #raise with a non-Exception non-String is not supported currently on TruffleRuby.Unfortunately it seems #642 broke on TruffleRuby and because of ruby/test-unit-ruby-core#23 it wasn't noticed.
In this PR I do an easy fix for this failure.
However after that the test suite still fails with a timeout.
I also tried increasing the timeout (from 10 to 100 commit) but it did not help.
When that test is run in isolation (
bundle exec ruby -Itest/lib -rhelper test/net/imap/test_imap_max_response_size.rb) it works fine though, so it sounds like there is some global state/side effect that makes the test fail somehow when run as part of the full test suite.I don't have time to investigate more now, maybe I can take another look in the evening.