<headius>
that should be pretty benign but there's not a lot of tests/specs checking these errors being raised during concurrent requires and such
<enebo[m]>
well that is not much to review but I guess I should read the comments
<headius>
I would just merge it but I am kinda form-fitting the fix to apparent behavior
<enebo[m]>
So I guess the risk of the fix is that it is unclear in edge cases if this is enough
<headius>
my reading of the CRuby code seems to indicate that they always remove this lock, but then they do not handle concurrency the same way we do
<enebo[m]>
but the basic problem reported is fixed and the rationale for the change makes sense
<headius>
removing the lock always for us broke the case where two requires are concurrent, the first one into the file raises an error (not LoadError) and that should let the second one acquire the same lock and continue loading
<enebo[m]>
We are early enough into 9.3 I think we should rolll with this
<headius>
ok I agree
<enebo[m]>
I wonder if there is some torture tests we can do on mixed failed autoloads and concurrent ones
<headius>
yeah it is tough to rig up because there's a lot of state invovled
<enebo[m]>
I don't really know how one would formulate a test like this but we obviously have a lot more edges than MRI
<enebo[m]>
I have to say so far so good on all that work you put into this to make zeitwork happy
<headius>
that's great
<enebo[m]>
on Ruby/spec I am down to 5e/f with 2 I cannot fix for Marshal#load. Getting much closer.
<headius>
cannot fix as in we cannot support the functionality?
<enebo[m]>
The 2 I cannot fix we could probably fix if we marked T_OBJECT/T_DATA on the few types which a T_DATA in MRI but it seems very unimportant
<headius>
right those ones
<headius>
they have been failing forever anyway
<enebo[m]>
1 test is it is invalid marshal data which tried to object unmarshal but it is a T_DATA type
<enebo[m]>
why someone would test this I don't know
<enebo[m]>
Seems like it should not be possible for this to happen unless you make the marshal string yourself
<headius>
yeah some of these date back to early rubinius days working on pure-Ruby marshal so I'm sure Evan or Brian pushed a lot of edges
<enebo[m]>
In this case I am unclear on when but MRI does actually type check on DATA/OBJECT in the code
<enebo[m]>
So I guess this could be something where you load data across Ruby versions where something switched types or something like that
<enebo[m]>
So I am not concerned
<headius>
does this test have some hardcoded marshaled data and tries to unmarshal that?
<enebo[m]>
I have not implemented a whole feature of marshal for some compat allocator thing
<enebo[m]>
yeah
<enebo[m]>
most of ruby/spec is like that
<headius>
or a related question, is this marshal data that 3.1 can actually produce
<enebo[m]>
There are some load/dump combo things but that presumes you have a working dump already
<headius>
if not then who cares, marshal data between Ruby versions is a massive risk anyway
<enebo[m]>
Which could harken back to Evan/Brian needing only half of it implemented
<enebo[m]>
We definitely had a number of issues. I am curious to see how I fare once I switch over to MRI tests
<headius>
second PR of mine tries to use native thread status for our thread status but it seems to hang on some tests so that needs more work:
<headius>
it was a quick attempt and I can iterate on that but this is trying to fix the issue where JI calls will not show up as "sleep" even if they bottom out in some blocking call
<headius>
I think that is worth finding a solution for but this needs work
<headius>
it might not be a 9.3 item
<headius>
changing the way we calculate thread status seems like too high a risk
<enebo[m]>
yeah this one would worry me a little but I am not having a lot of feels on risk
<enebo[m]>
other than it causing hangs :)
<enebo[m]>
headius: and IO scheduler seems like it might play into this
<headius>
yeah that will take some serious investigation to support around JI calls
<headius>
worst case if we wanted to try to support it we might have to put scheduler stuff around all JI calls, which would not really be worth the overhead
<headius>
it may be we have to say it can't be done for JI calls and if you want Ruby scheduler you need to use Ruby blocking calls
<headius>
given that this is not a regression and we have behaved the current way forever I'm going to punt to 9.4 and examine it along with scheduler work there
<headius>
that's a good point
<headius>
last one I don't know where to go with: using frame as target for nonlocal breaks
<headius>
I got everything working except for the case of a break in a Class.new {} body, which should break all the way to the level above the Class.new call
<headius>
the problem is that the module_eval logic clones the block and its frame before executing them to isolate changes they make while defining methods etc... at a minimum setting the frame class target for defs
<headius>
but that means the frame the block sees does not exist above the Class.new level so it can't target the break right
<headius>
it is a weird edge behavior but we pass it using DynamicScope as target
<enebo[m]>
hmm module_eval as a string or a block?
<headius>
p Class.new { break 1 } should print 1, but with my patch it LJEs
<headius>
the reason I was attempting to switch to frame was because it would be easier to mark them as being from a specific thread, to fix this issue:
<headius>
or at least it was a way to fix it without walking the thread's stack looking for a jump target
<enebo[m]>
yeah
<headius>
I could just see that the target was from another thread and raise LJE immediately
<headius>
this might still be fixable in module_eval but I'm not sure how... the block has to point at a frame with the Class.new object, but also target another frame for the break
<headius>
the logic in CRuby is pretty tangled as you might expect
<enebo[m]>
yeah this feels like a classic issue with blocks
<enebo[m]>
this feels like proc vs lambda where you have the same thing but different behavior
<enebo[m]>
as in you can have the same literal piece of code be both
<headius>
so that brings us back to the option of flagging DynScope as being from a specific thread, making them bigger and having more state to set up
<enebo[m]>
Or maybe another object altogether?
<headius>
hmm
<enebo[m]>
which I guess would need to be attached to something
<headius>
yeah I'm trying to think now if there's a way to just flag the block itself as having come from another thread
<headius>
without adding state to dynscope
<headius>
like on Binding or something
<headius>
then it just needs to look at its own Binding, see that it is from a foreign thread, and say nope to nonlocal break
<headius>
ok I can play with that idea
<headius>
making binding bigger is still cumbersome but it is already pretty big
<headius>
one more object reference, no additional alloc
<headius>
this level of change might again be too risky for 9.3
<headius>
I'm not sure
<enebo[m]>
I am getting a bogus marshal type byte of 6
<enebo[m]>
it is reading a none-ascii ivar so this has to be some weird issue of us not reading the whole string?
<enebo[m]>
I am getting so close
<headius>
last little tidbit on strscan... it passes tests, builds the gem ok I think, but because we have no existing JRuby that uses it as a gem I'm not sure how best to test it... working through that now
<enebo[m]>
Can we release 9.4+ and have SNAPSHOT work?
<headius>
it's kinda a chicken/egg thing
<headius>
I can't make snapshot use the gem because current gem installs a C ext
<headius>
and I can't release java gem without being able to test against snapshot
<headius>
maybe it will be acceptable that for now it is actually testing against the strscan in the snapshot build?
<enebo[m]>
well like a foreign key constraint you could remove the version check just for the sake of testing
<headius>
that's the problem basically... it loads strscan pretty early so it doesn't use the gem's jar even if we force it to
<headius>
so we have to make a hard break there unlike io-wait
<headius>
io-wait isn't loaded until later so the gem could activate
<headius>
this will be an issue with stringio as well most likely
<headius>
Yeah I'm surprised we didn't run into this working on Unicode identifiers
<enebo[m]>
yeah it assumes base encoding for result string and does not toggle it to proper one
<headius>
Must just be missing some logic for it to encode the Unicode as escaped characters
<enebo[m]>
// FIXME: bytelist_love: EPICLY wrong but something in MRI gets around identifiers of arbitrary encoding.
<enebo[m]>
HAHA
<enebo[m]>
Well past self Tom is trying to send present self Tom a message
<headius>
At least you knew we were broken
<enebo[m]>
And now I know it again
<enebo[m]>
ok well part of me feels strCat19 was the answer (if we still call it that) but why did past Tom not do that
<headius>
Yeah I'm sure we have logic for presenting inspected unicode string output so it should be the same thing
<headius>
Inspect output is supposed to always be ascii
<enebo[m]>
This should just change entire string to UTF-8 I am sure
<enebo[m]>
but you can have any encoding as an ivar
<enebo[m]>
So it must escape those
<headius>
Yeah it's just an inspect requirement that it never have non-7-bit ASCII I believe
<headius>
Never output
<enebo[m]>
yeah I can see it definitely is UTF-8 on MRI for this
<enebo[m]>
HAHA and ASCII-8BIT when US-ASCII only bytes?
<enebo[m]>
I never realized this but we are outputting as US-ASCII
<headius>
Inspect doesn't output ascii-8bit does it?
<enebo[m]>
we don't no but MRI does
<enebo[m]>
which in this case would also be wrong since it converts to UTF-8
<headius>
That's weird. I thought for sure they never output any bytes outside 7-bit range so why would it be ASCII 8-bit
<enebo[m]>
I believe it is the default since ivars can be multiple incompatible encodings perhaps
<enebo[m]>
I was not aware of this either though
<enebo[m]>
I am just observing inspected instances seems to be ascii-8bit by default back to at least 2.6
<enebo[m]>
These issues always amuse me because 99% of all classes are using 7bit and this is for inspect which most people do not use for useful data in a program (other than debugging/logging)
<enebo[m]>
It is just uncommon enough for no one to notice for years
<enebo[m]>
Pretty much a text book example of the difficulties of compatibility projects
<headius>
Yeah I guess we should add some specs for this
<headius>
You're right though, it is amazing how much coverage you really need for a language implementation like this
<headius>
Or maybe it's amazing how much behavior never gets exercised by any tests or specs or apps but it's still there as a compatibility challenge
<headius>
We probably should do a pass over encoding related MRI tests, not sure the last time we did that
subbu has quit [Remote host closed the connection]
subbu has joined #jruby
<byteit101[m]>
So I'm fine with convert (or perhaps `store_as:` ?) and de-nesting. I had created that to make illegal states unrepresentable, though I'm fine throwing exceptions instead.
<byteit101[m]>
But I notice you are preferring the actionless configuration name for the main config
<byteit101[m]>
Any particular reasons why?
<byteit101[m]>
I'd been preferring reading to writing in my thoughts, and as enebo mentioned with the looks like you could use :@othervariable as an argument had been disuading me from it (or variants) without a modifier word
<byteit101[m]>
With just instance_variable I feel like it could easily be misinterpreted as to what's going on
<byteit101[m]>
which is why I'd been preferring action words prefixing/suffixing (also for point #3 on my list about changing the semantics)
<byteit101[m]>
I do like headius's comparisoin to i_v_set and i_v_get which makes me lean towards instance_variable_bind
<byteit101[m]>
(or link, or something like that)
<byteit101[m]>
If someone who hasn't encountered this before reads the non-action {pre,post}fixed config I worry the semantics may not be as clear as if it were
<byteit101[m]>
thoughts? (I'll make a comment on the PR later this evening)
<headius>
6F7E on stringio
<headius>
looks like some missing kwargs for most of these